From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Add contrib/pg_logicalsnapinspect |
Date: | 2024-10-11 13:17:58 |
Message-ID: | Zwklhj/9vVxKGGqE@ip-10-97-1-34.eu-west-3.compute.internal |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On Fri, Oct 11, 2024 at 12:59:33PM +1100, Peter Smith wrote:
> Hi, Here are a few comments for patch set v13*
Thanks for looking at it.
> //////////
>
> Patch v13-0001
>
> ======
> Commit message
>
> 1.1
> /were no use case/was no use case/
Updated in v14 just shared up-thread.
> ~~~
>
> 1.2
> It seemed a bit odd that the switch cases for
> 'construct_array_builtin' are not the same as those for
> 'deconstruct_array_builtin'.
>
> For example, all these ones seem missing from deconstruct:
> case INT4OID:
> case INT8OID:
> case NAMEOID:
> case REGTYPEOID:
>
> I know that has nothing to do with your patch, and I guess it does not
> cause any problems otherwise there would be ERRORs. But, if you are to
> follow this same current pattern, then perhaps you don't need to add
> your new case for 'deconstruct_array_builtin', since AFAICT you are
> never using it.
That's right. Strict pairing between deconstruct_array_builtin() and
construct_array_builtin() is not required, let's remove this extra switch in
deconstruct_array_builtin() for code consistency (done in v14).
> //////////
>
> Patch v13-0002
>
> ======
> pg_get_logical_snapshot_meta:
>
> 2.1
> + Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
> + bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
>
> FWIW, if you wanted to avoid a few lines you could initialise the
> nulls array during the declaration.
> bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
>
> This seems a common pattern in other source code, and it replaces the
> need for the subsequent memset.
Okay, fine by me, let's do it for the "values" too in passing (this seems also
a common pattern), in v14.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Korotkov | 2024-10-11 13:20:32 | Re: POC, WIP: OR-clause support for indexes |
Previous Message | Laurenz Albe | 2024-10-11 13:16:52 | Re: On disable_cost |