Re: Add contrib/pg_logicalsnapinspect - Mailing list pgsql-hackers

Hi,

On Fri, Oct 11, 2024 at 04:48:26PM -0700, Masahiko Sawada wrote:
> On Fri, Oct 11, 2024 at 11:15 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > 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.

Thanks for the feedback. That makes sense and I agree with the proposal done
in v15.

> >
> > 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"

Yeah, I used it as a "template".

> but I personally think that "Function" is more appropriate
> like other modules does.

I do agree.

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

Agree.

Regards,

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