Re: Add contrib/pg_logicalsnapinspect

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