Horton

Re: Add contrib/pg_logicalsnapinspect

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

View thread
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, shveta malik <shveta.malik@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-10-11T13:17:58Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix indentation issue

  2. pg_logicalinspect: Stabilize isolation tests.

  3. Add contrib/pg_logicalinspect.

  4. Move SnapBuild and SnapBuildOnDisk structs to snapshot_internal.h.

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