Date: Mon, 14 Oct 2024 06:12:32 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Missing deconstruct_array_builtin usage
Message-ID: <Zwy2UK0yz2bbpyZp@ip-10-97-1-34.eu-west-3.compute.internal>
References: <Zwi5g2GzlUX1NqxR@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoCERkwmttY44dqUw=m_9QCctu7W+p6B7w_VqxRJA1Qq_Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="Fkzmc81Qd8vWzf7G"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAD21AoCERkwmttY44dqUw=m_9QCctu7W+p6B7w_VqxRJA1Qq_Q@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Zwy2UK0yz2bbpyZp%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--Fkzmc81Qd8vWzf7G
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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

--Fkzmc81Qd8vWzf7G
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-construct_array_builtin-usage-for-FLOAT8OID.patch"

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


--Fkzmc81Qd8vWzf7G--