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: 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-09-24T16:51:25Z
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.

Attachments

Hi,

On Mon, Sep 23, 2024 at 12:27:27PM +1000, Peter Smith wrote:
> My review comments for v8-0001
> 
> ======
> contrib/pg_logicalinspect/pg_logicalinspect.c
> 
> 1.
> +/*
> + * Lookup table for SnapBuildState.
> + */
> +
> +#define SNAPBUILD_STATE_INCR 1
> +
> +static const char *const SnapBuildStateDesc[] = {
> + [SNAPBUILD_START + SNAPBUILD_STATE_INCR] = "start",
> + [SNAPBUILD_BUILDING_SNAPSHOT + SNAPBUILD_STATE_INCR] = "building",
> + [SNAPBUILD_FULL_SNAPSHOT + SNAPBUILD_STATE_INCR] = "full",
> + [SNAPBUILD_CONSISTENT + SNAPBUILD_STATE_INCR] = "consistent",
> +};
> +
> +/*
> 
> nit - the SNAPBUILD_STATE_INCR made this code appear more complicated
> than it is. Please take a look at the attachment for an alternative
> implementation which includes an explanatory comment. YMMV. Feel free
> to ignore it.

Thanks for the feedback!

I like the commment, so added it in v9 attached. OTOH I think that's better
to keep SNAPBUILD_STATE_INCR as those "+1" are all linked and that would be
easy to miss the one in pg_get_logical_snapshot_info() should we change the
increment in the future.

> ======
> src/include/replication/snapbuild.h
> 
> 2.
> + * Please keep SnapBuildStateDesc[] (located in the pg_logicalinspect module)
> + * updated should a change needs to be done in SnapBuildState.
> 
> nit - "...should a change needs to be done" -- the word "needs" is
> incorrect here.
> 
> How about:
> "...if a change needs to be made to SnapBuildState."

Thanks, used this one in v9.

[1]: https://www.postgresql.org/message-id/CAJpy0uCppUNdod4F3NaPpMCtrySdw1S0T1d8CA-2c4CX%3DShMOQ%40mail.gmail.com

Regards,

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