Horton

Thread

Commits

  1. Use construct_array_builtin for FLOAT8OID instead of construct_array.

  2. Use deconstruct_array_builtin instead of deconstruct_array

  1. Missing deconstruct_array_builtin usage

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-10-11T05:37:07Z

    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.

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]).

[1]: https://www.postgresql.org/message-id/ZwdK1UYdLn/zFMHy%40ip-10-97-1-34.eu-west-3.compute.internal
[2]: https://www.postgresql.org/message-id/2914356f-9e5f-8c59-2995-5997fc48bcba%40enterprisedb.com

Looking forward to your feedback,

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  2. Re: Missing deconstruct_array_builtin usage

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-10-12T00:43:04Z

    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

  3. Re: Missing deconstruct_array_builtin usage

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-10-14T06:12:32Z

    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

  4. Re: Missing deconstruct_array_builtin usage

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-14T10:05:18Z

    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.

  5. Re: Missing deconstruct_array_builtin usage

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-10-14T16:55:24Z

    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