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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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
