Re: Add contrib/pg_logicalsnapinspect

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?

Regards,

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