Horton

Re: Add contrib/pg_logicalsnapinspect

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: Bertrand Drouvot <bertranddrouvot.pg@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-24T23:42:01Z
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.

On Wed, Sep 25, 2024 at 2:51 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> 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.
>

I see SNAPBUILD_STATE_INCR more as an "offset" (to get the lowest enum
value to be at lookup index [0]) than an "increment" (between the enum
values), so I'd be naming that differently. But, maybe I am straying
into just personal opinion instead of giving useful feedback, so let's
say I have no more review comments. Patch v9 looks OK to me.

======
Kind Regards,
Peter Smith.
Fujitsu Australia