On 1/16/18 01:14, Michael Paquier wrote: >> I don't see it either. I think the actually useful parts of that patch >> were to declare record_image_cmp's result correctly as int rather than >> bool, and to cope with varlena fields of unequal size. Unfortunately >> there seems to be no contemporaneous mailing list discussion, so >> it's not clear what Kevin based this change on. > > This was a hotfix after a buildfarm breakage, the base commit being > f566515. > >> Want to try reverting the GET_X_BYTES() parts of it and see if the >> buildfarm complains? > > So, Peter, are you planning to do so?
Here is a proposed patch set to clean this up. First, add some test coverage for record_image_cmp. (There wasn't any, only for record_image_eq as part of MV testing.) Then, remove the GET_ macros from record_image_{eq,cmp}. And finally remove all that byte-masking stuff from postgres.h. >> Note if that if we simplify the GetDatum macros, it's possible that >> record_image_cmp would change behavior, since it might now see signed not >> unsigned values depending on whether the casts do sign extension or not. >> Not sure if that'd be a problem. I wasn't able to construct such a case. Everything still works unsigned end-to-end, I think. But if you can think of a case, we can throw it into the tests. The tests already contain cases of comparing positive and negative integers. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 686b2a6f2c0a455dccbecf07d163af5d6f9c9e88 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pete...@gmx.net> Date: Tue, 23 Jan 2018 10:13:45 -0500 Subject: [PATCH 1/3] Add tests for record_image_eq and record_image_cmp record_image_eq was covered a bit by the materialized view code that it is meant to support, but record_image_cmp was not tested at all. --- src/test/regress/expected/rowtypes.out | 161 +++++++++++++++++++++++++++++++++ src/test/regress/sql/rowtypes.sql | 53 +++++++++++ 2 files changed, 214 insertions(+) diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out index a4bac8e3b5..e3c23a41cd 100644 --- a/src/test/regress/expected/rowtypes.out +++ b/src/test/regress/expected/rowtypes.out @@ -369,6 +369,167 @@ LINE 1: select * from cc order by f1; ^ HINT: Use an explicit ordering operator or modify the query. -- +-- Tests for record_image_{eq,cmp} +-- +create type testtype1 as (a int, b int); +-- all true +select row(1, 2)::testtype1 *< row(1, 3)::testtype1; + ?column? +---------- + t +(1 row) + +select row(1, 2)::testtype1 *<= row(1, 3)::testtype1; + ?column? +---------- + t +(1 row) + +select row(1, 2)::testtype1 *= row(1, 2)::testtype1; + ?column? +---------- + t +(1 row) + +select row(1, 2)::testtype1 *<> row(1, 3)::testtype1; + ?column? +---------- + t +(1 row) + +select row(1, 3)::testtype1 *>= row(1, 2)::testtype1; + ?column? +---------- + t +(1 row) + +select row(1, 3)::testtype1 *> row(1, 2)::testtype1; + ?column? +---------- + t +(1 row) + +-- all false +select row(1, -2)::testtype1 *< row(1, -3)::testtype1; + ?column? +---------- + f +(1 row) + +select row(1, -2)::testtype1 *<= row(1, -3)::testtype1; + ?column? +---------- + f +(1 row) + +select row(1, -2)::testtype1 *= row(1, -3)::testtype1; + ?column? +---------- + f +(1 row) + +select row(1, -2)::testtype1 *<> row(1, -2)::testtype1; + ?column? +---------- + f +(1 row) + +select row(1, -3)::testtype1 *>= row(1, -2)::testtype1; + ?column? +---------- + f +(1 row) + +select row(1, -3)::testtype1 *> row(1, -2)::testtype1; + ?column? +---------- + f +(1 row) + +-- This returns the "wrong" order because record_image_cmp works on +-- unsigned datums without knowing about the actual data type. +select row(1, -2)::testtype1 *< row(1, 3)::testtype1; + ?column? +---------- + f +(1 row) + +-- other types +create type testtype2 as (a smallint, b bool); -- byval different sizes +select row(1, true)::testtype2 *< row(2, true)::testtype2; + ?column? +---------- + t +(1 row) + +select row(-2, true)::testtype2 *< row(-1, true)::testtype2; + ?column? +---------- + t +(1 row) + +select row(0, false)::testtype2 *< row(0, true)::testtype2; + ?column? +---------- + t +(1 row) + +select row(0, false)::testtype2 *<> row(0, true)::testtype2; + ?column? +---------- + t +(1 row) + +create type testtype3 as (a int, b text); -- variable length +select row(1, 'abc')::testtype3 *< row(1, 'abd')::testtype3; + ?column? +---------- + t +(1 row) + +select row(1, 'abc')::testtype3 *< row(1, 'abcd')::testtype3; + ?column? +---------- + t +(1 row) + +select row(1, 'abc')::testtype3 *> row(1, 'abd')::testtype3; + ?column? +---------- + f +(1 row) + +select row(1, 'abc')::testtype3 *<> row(1, 'abd')::testtype3; + ?column? +---------- + t +(1 row) + +create type testtype4 as (a int, b point); -- by ref, fixed length +select row(1, '(1,2)')::testtype4 *< row(1, '(1,3)')::testtype4; + ?column? +---------- + t +(1 row) + +select row(1, '(1,2)')::testtype4 *<> row(1, '(1,3)')::testtype4; + ?column? +---------- + t +(1 row) + +-- mismatches +select row(1, 2)::testtype1 *< row(1, 'abc')::testtype3; +ERROR: cannot compare dissimilar column types integer and text at record column 2 +select row(1, 2)::testtype1 *<> row(1, 'abc')::testtype3; +ERROR: cannot compare dissimilar column types integer and text at record column 2 +create type testtype5 as (a int); +select row(1, 2)::testtype1 *< row(1)::testtype5; +ERROR: cannot compare record types with different numbers of columns +select row(1, 2)::testtype1 *<> row(1)::testtype5; +ERROR: cannot compare record types with different numbers of columns +drop type testtype1, testtype2, testtype3, testtype4, testtype5; +-- -- Test case derived from bug #5716: check multiple uses of a rowtype result -- BEGIN; diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql index 8d63060500..ef80af04aa 100644 --- a/src/test/regress/sql/rowtypes.sql +++ b/src/test/regress/sql/rowtypes.sql @@ -160,6 +160,59 @@ insert into cc values('("(4,5)",6)'); select * from cc order by f1; -- fail, but should complain about cantcompare +-- +-- Tests for record_image_{eq,cmp} +-- + +create type testtype1 as (a int, b int); + +-- all true +select row(1, 2)::testtype1 *< row(1, 3)::testtype1; +select row(1, 2)::testtype1 *<= row(1, 3)::testtype1; +select row(1, 2)::testtype1 *= row(1, 2)::testtype1; +select row(1, 2)::testtype1 *<> row(1, 3)::testtype1; +select row(1, 3)::testtype1 *>= row(1, 2)::testtype1; +select row(1, 3)::testtype1 *> row(1, 2)::testtype1; + +-- all false +select row(1, -2)::testtype1 *< row(1, -3)::testtype1; +select row(1, -2)::testtype1 *<= row(1, -3)::testtype1; +select row(1, -2)::testtype1 *= row(1, -3)::testtype1; +select row(1, -2)::testtype1 *<> row(1, -2)::testtype1; +select row(1, -3)::testtype1 *>= row(1, -2)::testtype1; +select row(1, -3)::testtype1 *> row(1, -2)::testtype1; + +-- This returns the "wrong" order because record_image_cmp works on +-- unsigned datums without knowing about the actual data type. +select row(1, -2)::testtype1 *< row(1, 3)::testtype1; + +-- other types +create type testtype2 as (a smallint, b bool); -- byval different sizes +select row(1, true)::testtype2 *< row(2, true)::testtype2; +select row(-2, true)::testtype2 *< row(-1, true)::testtype2; +select row(0, false)::testtype2 *< row(0, true)::testtype2; +select row(0, false)::testtype2 *<> row(0, true)::testtype2; + +create type testtype3 as (a int, b text); -- variable length +select row(1, 'abc')::testtype3 *< row(1, 'abd')::testtype3; +select row(1, 'abc')::testtype3 *< row(1, 'abcd')::testtype3; +select row(1, 'abc')::testtype3 *> row(1, 'abd')::testtype3; +select row(1, 'abc')::testtype3 *<> row(1, 'abd')::testtype3; + +create type testtype4 as (a int, b point); -- by ref, fixed length +select row(1, '(1,2)')::testtype4 *< row(1, '(1,3)')::testtype4; +select row(1, '(1,2)')::testtype4 *<> row(1, '(1,3)')::testtype4; + +-- mismatches +select row(1, 2)::testtype1 *< row(1, 'abc')::testtype3; +select row(1, 2)::testtype1 *<> row(1, 'abc')::testtype3; +create type testtype5 as (a int); +select row(1, 2)::testtype1 *< row(1)::testtype5; +select row(1, 2)::testtype1 *<> row(1)::testtype5; + +drop type testtype1, testtype2, testtype3, testtype4, testtype5; + + -- -- Test case derived from bug #5716: check multiple uses of a rowtype result -- base-commit: b3f8401205afdaf63cb20dc316d44644c933d5a1 -- 2.16.1
From 90df2e3d606d9c006f16d1cbc9066ab571dd3708 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pete...@gmx.net> Date: Tue, 23 Jan 2018 10:55:40 -0500 Subject: [PATCH 2/3] Remove use of byte-masking macros in record_image_cmp These were introduced in 4cbb646334b3b998a29abef0d57608d42097e6c9, but after further analysis, they should not be necessary and probably weren't the part of that commit that fixed anything. --- src/backend/utils/adt/rowtypes.c | 65 ++-------------------------------------- 1 file changed, 3 insertions(+), 62 deletions(-) diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c index a5fabfcc9e..5f729342f8 100644 --- a/src/backend/utils/adt/rowtypes.c +++ b/src/backend/utils/adt/rowtypes.c @@ -1467,45 +1467,8 @@ record_image_cmp(FunctionCallInfo fcinfo) } else if (att1->attbyval) { - switch (att1->attlen) - { - case 1: - if (GET_1_BYTE(values1[i1]) != - GET_1_BYTE(values2[i2])) - { - cmpresult = (GET_1_BYTE(values1[i1]) < - GET_1_BYTE(values2[i2])) ? -1 : 1; - } - break; - case 2: - if (GET_2_BYTES(values1[i1]) != - GET_2_BYTES(values2[i2])) - { - cmpresult = (GET_2_BYTES(values1[i1]) < - GET_2_BYTES(values2[i2])) ? -1 : 1; - } - break; - case 4: - if (GET_4_BYTES(values1[i1]) != - GET_4_BYTES(values2[i2])) - { - cmpresult = (GET_4_BYTES(values1[i1]) < - GET_4_BYTES(values2[i2])) ? -1 : 1; - } - break; -#if SIZEOF_DATUM == 8 - case 8: - if (GET_8_BYTES(values1[i1]) != - GET_8_BYTES(values2[i2])) - { - cmpresult = (GET_8_BYTES(values1[i1]) < - GET_8_BYTES(values2[i2])) ? -1 : 1; - } - break; -#endif - default: - Assert(false); /* cannot happen */ - } + if (values1[i1] != values2[i2]) + cmpresult = (values1[i1] < values2[i2]) ? -1 : 1; } else { @@ -1739,29 +1702,7 @@ record_image_eq(PG_FUNCTION_ARGS) } else if (att1->attbyval) { - switch (att1->attlen) - { - case 1: - result = (GET_1_BYTE(values1[i1]) == - GET_1_BYTE(values2[i2])); - break; - case 2: - result = (GET_2_BYTES(values1[i1]) == - GET_2_BYTES(values2[i2])); - break; - case 4: - result = (GET_4_BYTES(values1[i1]) == - GET_4_BYTES(values2[i2])); - break; -#if SIZEOF_DATUM == 8 - case 8: - result = (GET_8_BYTES(values1[i1]) == - GET_8_BYTES(values2[i2])); - break; -#endif - default: - Assert(false); /* cannot happen */ - } + result = (values1[i1] == values2[i2]); } else { -- 2.16.1
From da0b315dccb90a74f2dba7d65d4640f9938c8bda Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pete...@gmx.net> Date: Tue, 23 Jan 2018 11:19:12 -0500 Subject: [PATCH 3/3] Remove byte-masking macros for Datum conversion macros As the comment there stated, these were needed for old-style user-defined functions, but since we removed support for those, we don't need this anymore. --- src/include/postgres.h | 86 +++++++++++++++++++------------------------------- 1 file changed, 32 insertions(+), 54 deletions(-) diff --git a/src/include/postgres.h b/src/include/postgres.h index b69f88aa5b..514c65edc1 100644 --- a/src/include/postgres.h +++ b/src/include/postgres.h @@ -349,54 +349,32 @@ typedef struct /* ---------------------------------------------------------------- - * Section 2: datum type + support macros + * Section 2: Datum type + support macros * ---------------------------------------------------------------- */ /* - * Port Notes: - * Postgres makes the following assumptions about datatype sizes: + * A Datum contains either a value of a pass-by-value type or a pointer to a + * value of a pass-by-reference type. Therefore, we require: * - * sizeof(Datum) == sizeof(void *) == 4 or 8 - * sizeof(char) == 1 - * sizeof(short) == 2 + * sizeof(Datum) == sizeof(void *) == 4 or 8 * - * When a type narrower than Datum is stored in a Datum, we place it in the - * low-order bits and are careful that the DatumGetXXX macro for it discards - * the unused high-order bits (as opposed to, say, assuming they are zero). - * This is needed to support old-style user-defined functions, since depending - * on architecture and compiler, the return value of a function returning char - * or short may contain garbage when called as if it returned Datum. + * The macros below and the analogous macros for other types should be used to + * convert between a Datum and the appropriate C type. */ typedef uintptr_t Datum; #define SIZEOF_DATUM SIZEOF_VOID_P -typedef Datum *DatumPtr; - -#define GET_1_BYTE(datum) (((Datum) (datum)) & 0x000000ff) -#define GET_2_BYTES(datum) (((Datum) (datum)) & 0x0000ffff) -#define GET_4_BYTES(datum) (((Datum) (datum)) & 0xffffffff) -#if SIZEOF_DATUM == 8 -#define GET_8_BYTES(datum) ((Datum) (datum)) -#endif -#define SET_1_BYTE(value) (((Datum) (value)) & 0x000000ff) -#define SET_2_BYTES(value) (((Datum) (value)) & 0x0000ffff) -#define SET_4_BYTES(value) (((Datum) (value)) & 0xffffffff) -#if SIZEOF_DATUM == 8 -#define SET_8_BYTES(value) ((Datum) (value)) -#endif - /* * DatumGetBool * Returns boolean value of a datum. * - * Note: any nonzero value will be considered TRUE, but we ignore bits to - * the left of the width of bool, per comment above. + * Note: any nonzero value will be considered TRUE. */ -#define DatumGetBool(X) ((bool) (GET_1_BYTE(X) != 0)) +#define DatumGetBool(X) ((bool) ((X) != 0)) /* * BoolGetDatum @@ -412,140 +390,140 @@ typedef Datum *DatumPtr; * Returns character value of a datum. */ -#define DatumGetChar(X) ((char) GET_1_BYTE(X)) +#define DatumGetChar(X) ((char) (X)) /* * CharGetDatum * Returns datum representation for a character. */ -#define CharGetDatum(X) ((Datum) SET_1_BYTE(X)) +#define CharGetDatum(X) ((Datum) (X)) /* * Int8GetDatum * Returns datum representation for an 8-bit integer. */ -#define Int8GetDatum(X) ((Datum) SET_1_BYTE(X)) +#define Int8GetDatum(X) ((Datum) (X)) /* * DatumGetUInt8 * Returns 8-bit unsigned integer value of a datum. */ -#define DatumGetUInt8(X) ((uint8) GET_1_BYTE(X)) +#define DatumGetUInt8(X) ((uint8) (X)) /* * UInt8GetDatum * Returns datum representation for an 8-bit unsigned integer. */ -#define UInt8GetDatum(X) ((Datum) SET_1_BYTE(X)) +#define UInt8GetDatum(X) ((Datum) (X)) /* * DatumGetInt16 * Returns 16-bit integer value of a datum. */ -#define DatumGetInt16(X) ((int16) GET_2_BYTES(X)) +#define DatumGetInt16(X) ((int16) (X)) /* * Int16GetDatum * Returns datum representation for a 16-bit integer. */ -#define Int16GetDatum(X) ((Datum) SET_2_BYTES(X)) +#define Int16GetDatum(X) ((Datum) (X)) /* * DatumGetUInt16 * Returns 16-bit unsigned integer value of a datum. */ -#define DatumGetUInt16(X) ((uint16) GET_2_BYTES(X)) +#define DatumGetUInt16(X) ((uint16) (X)) /* * UInt16GetDatum * Returns datum representation for a 16-bit unsigned integer. */ -#define UInt16GetDatum(X) ((Datum) SET_2_BYTES(X)) +#define UInt16GetDatum(X) ((Datum) (X)) /* * DatumGetInt32 * Returns 32-bit integer value of a datum. */ -#define DatumGetInt32(X) ((int32) GET_4_BYTES(X)) +#define DatumGetInt32(X) ((int32) (X)) /* * Int32GetDatum * Returns datum representation for a 32-bit integer. */ -#define Int32GetDatum(X) ((Datum) SET_4_BYTES(X)) +#define Int32GetDatum(X) ((Datum) (X)) /* * DatumGetUInt32 * Returns 32-bit unsigned integer value of a datum. */ -#define DatumGetUInt32(X) ((uint32) GET_4_BYTES(X)) +#define DatumGetUInt32(X) ((uint32) (X)) /* * UInt32GetDatum * Returns datum representation for a 32-bit unsigned integer. */ -#define UInt32GetDatum(X) ((Datum) SET_4_BYTES(X)) +#define UInt32GetDatum(X) ((Datum) (X)) /* * DatumGetObjectId * Returns object identifier value of a datum. */ -#define DatumGetObjectId(X) ((Oid) GET_4_BYTES(X)) +#define DatumGetObjectId(X) ((Oid) (X)) /* * ObjectIdGetDatum * Returns datum representation for an object identifier. */ -#define ObjectIdGetDatum(X) ((Datum) SET_4_BYTES(X)) +#define ObjectIdGetDatum(X) ((Datum) (X)) /* * DatumGetTransactionId * Returns transaction identifier value of a datum. */ -#define DatumGetTransactionId(X) ((TransactionId) GET_4_BYTES(X)) +#define DatumGetTransactionId(X) ((TransactionId) (X)) /* * TransactionIdGetDatum * Returns datum representation for a transaction identifier. */ -#define TransactionIdGetDatum(X) ((Datum) SET_4_BYTES((X))) +#define TransactionIdGetDatum(X) ((Datum) (X)) /* * MultiXactIdGetDatum * Returns datum representation for a multixact identifier. */ -#define MultiXactIdGetDatum(X) ((Datum) SET_4_BYTES((X))) +#define MultiXactIdGetDatum(X) ((Datum) (X)) /* * DatumGetCommandId * Returns command identifier value of a datum. */ -#define DatumGetCommandId(X) ((CommandId) GET_4_BYTES(X)) +#define DatumGetCommandId(X) ((CommandId) (X)) /* * CommandIdGetDatum * Returns datum representation for a command identifier. */ -#define CommandIdGetDatum(X) ((Datum) SET_4_BYTES(X)) +#define CommandIdGetDatum(X) ((Datum) (X)) /* * DatumGetPointer @@ -608,7 +586,7 @@ typedef Datum *DatumPtr; */ #ifdef USE_FLOAT8_BYVAL -#define DatumGetInt64(X) ((int64) GET_8_BYTES(X)) +#define DatumGetInt64(X) ((int64) (X)) #else #define DatumGetInt64(X) (* ((int64 *) DatumGetPointer(X))) #endif @@ -622,7 +600,7 @@ typedef Datum *DatumPtr; */ #ifdef USE_FLOAT8_BYVAL -#define Int64GetDatum(X) ((Datum) SET_8_BYTES(X)) +#define Int64GetDatum(X) ((Datum) (X)) #else extern Datum Int64GetDatum(int64 X); #endif @@ -635,7 +613,7 @@ extern Datum Int64GetDatum(int64 X); */ #ifdef USE_FLOAT8_BYVAL -#define DatumGetUInt64(X) ((uint64) GET_8_BYTES(X)) +#define DatumGetUInt64(X) ((uint64) (X)) #else #define DatumGetUInt64(X) (* ((uint64 *) DatumGetPointer(X))) #endif @@ -649,7 +627,7 @@ extern Datum Int64GetDatum(int64 X); */ #ifdef USE_FLOAT8_BYVAL -#define UInt64GetDatum(X) ((Datum) SET_8_BYTES(X)) +#define UInt64GetDatum(X) ((Datum) (X)) #else #define UInt64GetDatum(X) Int64GetDatum((int64) (X)) #endif -- 2.16.1