Missing deconstruct_array_builtin usage
Hi hackers,
While working on [1]/messages/by-id/ZwdK1UYdLn/zFMHy@ip-10-97-1-34.eu-west-3.compute.internal, I noticed that we missed using deconstruct_array_builtin()
in 062a8444242.
Indeed, d746021de1 added construct_array_builtin and deconstruct_array_builtin
but , later on, 062a8444242 made use of deconstruct_array for TEXTOID.
Please find attached a tiny patch to add the $SUBJECT.
That does not fix any issues, it just removes this unnecessary hardcoded
parameters related to TEXTOID passed to deconstruct_array.
A quick check:
$ git grep construct_array | grep OID | grep -v builtin
src/backend/catalog/pg_publication.c: deconstruct_array(arr, TEXTOID, -1, false, TYPALIGN_INT,
src/backend/utils/fmgr/funcapi.c: * For the OID and char arrays, we don't need to use deconstruct_array()
shows that this is the "only" miss since d746021de1, so I still don't think it
has to be more "complicated" than it is currently (as already mentioned by Peter
in [2]/messages/by-id/2914356f-9e5f-8c59-2995-5997fc48bcba@enterprisedb.com).
[1]: /messages/by-id/ZwdK1UYdLn/zFMHy@ip-10-97-1-34.eu-west-3.compute.internal
[2]: /messages/by-id/2914356f-9e5f-8c59-2995-5997fc48bcba@enterprisedb.com
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Missing-deconstruct_array_builtin-usage.patchtext/x-diff; charset=us-asciiDownload
From 814d608c39766dc550b4622cb10edd3654ac1c3a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 11 Oct 2024 04:27:59 +0000
Subject: [PATCH v1] Missing deconstruct_array_builtin usage
d746021de1 added construct_array_builtin and deconstruct_array_builtin but
, later on, 062a8444242 made use of deconstruct_array for TEXTOID. Making use of
deconstruct_array_builtin instead. That does not fix any issues, it just removes
this unnecessary hardcoded parameters related to TEXTOID passed to
deconstruct_array.
---
src/backend/catalog/pg_publication.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
100.0% src/backend/catalog/
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 7fe5fe2b86..7e5e357fd9 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1059,8 +1059,7 @@ pg_get_publication_tables(PG_FUNCTION_ARGS)
* publication name.
*/
arr = PG_GETARG_ARRAYTYPE_P(0);
- deconstruct_array(arr, TEXTOID, -1, false, TYPALIGN_INT,
- &elems, NULL, &nelems);
+ deconstruct_array_builtin(arr, TEXTOID, &elems, NULL, &nelems);
/* Get Oids of tables from each publication. */
for (i = 0; i < nelems; i++)
--
2.34.1
Hi,
On Thu, Oct 10, 2024 at 10:37 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
Hi hackers,
While working on [1], I noticed that we missed using deconstruct_array_builtin()
in 062a8444242.Indeed, d746021de1 added construct_array_builtin and deconstruct_array_builtin
but , later on, 062a8444242 made use of deconstruct_array for TEXTOID.Please find attached a tiny patch to add the $SUBJECT.
That does not fix any issues, it just removes this unnecessary hardcoded
parameters related to TEXTOID passed to deconstruct_array.
+1 for better consistency and simplicity.
A quick check:
$ git grep construct_array | grep OID | grep -v builtin
p> src/backend/catalog/pg_publication.c:
deconstruct_array(arr, TEXTOID, -1, false, TYPALIGN_INT,
src/backend/utils/fmgr/funcapi.c: * For the OID and char arrays, we don't need to use deconstruct_array()
shows that this is the "only" miss since d746021de1, so I still don't think it
has to be more "complicated" than it is currently (as already mentioned by Peter
in [2]).
It seems that src/backend/utils/adt/float.c file still has functions
that can use [de]construct_array_builtin(), for example
float8_combine(). I think it's an oversight of d746021de1.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Hi,
On Fri, Oct 11, 2024 at 05:43:04PM -0700, Masahiko Sawada wrote:
Hi,
On Thu, Oct 10, 2024 at 10:37 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:Hi hackers,
While working on [1], I noticed that we missed using deconstruct_array_builtin()
in 062a8444242.Indeed, d746021de1 added construct_array_builtin and deconstruct_array_builtin
but , later on, 062a8444242 made use of deconstruct_array for TEXTOID.Please find attached a tiny patch to add the $SUBJECT.
That does not fix any issues, it just removes this unnecessary hardcoded
parameters related to TEXTOID passed to deconstruct_array.+1 for better consistency and simplicity.
A quick check:
$ git grep construct_array | grep OID | grep -v builtin
p> src/backend/catalog/pg_publication.c:
deconstruct_array(arr, TEXTOID, -1, false, TYPALIGN_INT,src/backend/utils/fmgr/funcapi.c: * For the OID and char arrays, we don't need to use deconstruct_array()
shows that this is the "only" miss since d746021de1, so I still don't think it
has to be more "complicated" than it is currently (as already mentioned by Peter
in [2]).It seems that src/backend/utils/adt/float.c file still has functions
that can use [de]construct_array_builtin(), for example
float8_combine(). I think it's an oversight of d746021de1.
Thanks for looking at it.
Good catch, please find attached v2 taking care of those. I looked at the
remaining [de]construct_array() usages and that looks ok to me.
Please note that v1 has been committed (099c572d33).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-construct_array_builtin-usage-for-FLOAT8OID.patchtext/x-diff; charset=us-asciiDownload
From 7227bc20a7d3863200129c13750089f642032332 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Sun, 13 Oct 2024 16:14:01 +0000
Subject: [PATCH v2] construct_array_builtin usage for FLOAT8OID
Commit d746021de1 introduced use of construct_array_builtin for built-in types
instead of construct_array but forgot some replacements linked to FLOAT8OID.
---
src/backend/utils/adt/arrayfuncs.c | 6 ++++++
src/backend/utils/adt/float.c | 20 +++++---------------
2 files changed, 11 insertions(+), 15 deletions(-)
100.0% src/backend/utils/adt/
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index e5c7e57a5d..1640d83885 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -3404,6 +3404,12 @@ construct_array_builtin(Datum *elems, int nelems, Oid elmtype)
elmalign = TYPALIGN_INT;
break;
+ case FLOAT8OID:
+ elmlen = sizeof(float8);
+ elmbyval = FLOAT8PASSBYVAL;
+ elmalign = TYPALIGN_DOUBLE;
+ break;
+
case INT2OID:
elmlen = sizeof(int16);
elmbyval = true;
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index f709c21e1f..6fa6ffb51f 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2946,9 +2946,7 @@ float8_combine(PG_FUNCTION_ARGS)
transdatums[1] = Float8GetDatumFast(Sx);
transdatums[2] = Float8GetDatumFast(Sxx);
- result = construct_array(transdatums, 3,
- FLOAT8OID,
- sizeof(float8), FLOAT8PASSBYVAL, TYPALIGN_DOUBLE);
+ result = construct_array_builtin(transdatums, 3, FLOAT8OID);
PG_RETURN_ARRAYTYPE_P(result);
}
@@ -3029,9 +3027,7 @@ float8_accum(PG_FUNCTION_ARGS)
transdatums[1] = Float8GetDatumFast(Sx);
transdatums[2] = Float8GetDatumFast(Sxx);
- result = construct_array(transdatums, 3,
- FLOAT8OID,
- sizeof(float8), FLOAT8PASSBYVAL, TYPALIGN_DOUBLE);
+ result = construct_array_builtin(transdatums, 3, FLOAT8OID);
PG_RETURN_ARRAYTYPE_P(result);
}
@@ -3114,9 +3110,7 @@ float4_accum(PG_FUNCTION_ARGS)
transdatums[1] = Float8GetDatumFast(Sx);
transdatums[2] = Float8GetDatumFast(Sxx);
- result = construct_array(transdatums, 3,
- FLOAT8OID,
- sizeof(float8), FLOAT8PASSBYVAL, TYPALIGN_DOUBLE);
+ result = construct_array_builtin(transdatums, 3, FLOAT8OID);
PG_RETURN_ARRAYTYPE_P(result);
}
@@ -3359,9 +3353,7 @@ float8_regr_accum(PG_FUNCTION_ARGS)
transdatums[4] = Float8GetDatumFast(Syy);
transdatums[5] = Float8GetDatumFast(Sxy);
- result = construct_array(transdatums, 6,
- FLOAT8OID,
- sizeof(float8), FLOAT8PASSBYVAL, TYPALIGN_DOUBLE);
+ result = construct_array_builtin(transdatums, 6, FLOAT8OID);
PG_RETURN_ARRAYTYPE_P(result);
}
@@ -3500,9 +3492,7 @@ float8_regr_combine(PG_FUNCTION_ARGS)
transdatums[4] = Float8GetDatumFast(Syy);
transdatums[5] = Float8GetDatumFast(Sxy);
- result = construct_array(transdatums, 6,
- FLOAT8OID,
- sizeof(float8), FLOAT8PASSBYVAL, TYPALIGN_DOUBLE);
+ result = construct_array_builtin(transdatums, 6, FLOAT8OID);
PG_RETURN_ARRAYTYPE_P(result);
}
--
2.34.1
On 14.10.24 08:12, Bertrand Drouvot wrote:
It seems that src/backend/utils/adt/float.c file still has functions
that can use [de]construct_array_builtin(), for example
float8_combine(). I think it's an oversight of d746021de1.Thanks for looking at it.
Good catch, please find attached v2 taking care of those. I looked at the
remaining [de]construct_array() usages and that looks ok to me.
This looks good to me. I can't think of a reason why it was not
included in the original patch.
On Mon, Oct 14, 2024 at 3:05 AM Peter Eisentraut <peter@eisentraut.org> wrote:
On 14.10.24 08:12, Bertrand Drouvot wrote:
It seems that src/backend/utils/adt/float.c file still has functions
that can use [de]construct_array_builtin(), for example
float8_combine(). I think it's an oversight of d746021de1.Thanks for looking at it.
Good catch, please find attached v2 taking care of those. I looked at the
remaining [de]construct_array() usages and that looks ok to me.This looks good to me. I can't think of a reason why it was not
included in the original patch.
Thank you for reviewing the patch. Pushed (v2 patch).
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com