Horton

Re: Add contrib/pg_logicalsnapinspect

Masahiko Sawada <sawada.mshk@gmail.com>

View thread
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Peter Smith <smithpb2250@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-11T18:15:46Z
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 Fri, Oct 11, 2024 at 6:15 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Thu, Oct 10, 2024 at 05:38:43PM -0700, Masahiko Sawada wrote:
> > On Thu, Oct 10, 2024 at 6:10 AM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> >
> > The patches mostly look good to me. Here are some minor comments:
>
> Thanks for looking at it!
>
> >
> > +       sprintf(path, "%s/%s",
> > +                       PG_LOGICAL_SNAPSHOTS_DIR,
> > +                       text_to_cstring(filename_t));
> > +
> > +       /* Validate and restore the snapshot to 'ondisk' */
> > +       ValidateAndRestoreSnapshotFile(&ondisk, path,
> > CurrentMemoryContext, false);
> > +
> > +       /* Build a tuple descriptor for our result type */
> > +       if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> > +               elog(ERROR, "return type must be a row type");
> > +
> > I think it would be better to check the result type before reading the
> > snapshot file.
>
> Agree, done in v14.
>
> >
> > ---
> > +       values[i++] = Int64GetDatum((int64) ondisk.checksum);
> >
> > Why is only checksum casted to int64? With that, it can show a
> > checksum value as a non-netagive integer but is it really necessary?
> > For instance, page_header() function in pageinspect shows a page
> > checksum as smallint.
>
> Yeah, pd_checksum in PageHeaderData is uint16 while checksum in SnapBuildOnDisk
> is pg_crc32c. The reason why it is casted to int64 is explained in [1], does that
> make sense to you?

In the email, you said:

> As the checksum could be > 2^31 - 1, then v9 (just shared up-thread) changes it
> to an int8 in the pg_logicalinspect--1.0.sql file. So, to avoid CI failure on
> the 32bit build, then v9 is using Int64GetDatum() instead of UInt32GetDatum().

I'm fine with using Int64GetDatum() for checksum.

>
> > Same goes for below:
> > values[i++] = Int32GetDatum(ondisk.magic);
> > values[i++] = Int32GetDatum(ondisk.magic);
>
> The 2 others field (magic and version) are unlikely to be > 2^31 - 1, so v9 is
> making use of UInt32GetDatum() and keep int4 in the sql file.

While I agree that these two fields are unlikely to be > 2^31 - 1, I'm
concerned a bit about an inconsistency that the patch uses
Int64GetDatum also for both ondisk.builder.committed.xcnt and
ondisk.builder.catchange.xcnt.

I have a minor comment:

+ <sect2 id="pglogicalinspect-funcs">
+  <title>General Functions</title>

If we use "General Functions" here it sounds like there are other
functions for specific purposes in pg_logicalinspect module. How about
using "Functions" instead?

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com