Re: Add contrib/pg_logicalsnapinspect

On Fri, Oct 11, 2024 at 11:15 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Fri, Oct 11, 2024 at 6:15 AM Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)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(dot)pg(at)gmail(dot)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?

To elaborate further, pageinspect has a "General Functions" section,
which makes sense to me as it has other AM-type specific functions. On
the other hand, pg_logicalinspect has SQL functions only for one
logical replication component. So I think it makes sense to use
"Function" instead. pg_walinspect also has the sole section "General
Function" but I personally think that "Function" is more appropriate
like other modules does.

BTW I think that adding snapshot_internal.h could be a separate patch.
That makes the main pg_logicalinspect patch cleaner.

I've attached updated patches with some minor changes, and done the patch split.

The 0001 patch just moves both SnapBuild and SnapBuildOnDisk structs
to snapshot_internal.h. The 0002 is the main pg_logicalinspect patch.
I've merged the previous Add-XIDOID-in-construct_array_builtin.patch
to the main patch. The minor_change.patch.txt is the difference I
added on top of v14 patch set.

I'm going to push them early next week, barring any objections and
further comments.

Regards,

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