Date: Mon, 14 Oct 2024 06:17:38 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Masahiko Sawada <sawada.mshk@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
Subject: Re: Add contrib/pg_logicalsnapinspect
Message-ID: <Zwy3goasKiSBQeEe@ip-10-97-1-34.eu-west-3.compute.internal>
References: <CAD21AoAO3zyooPQ+E2rpEi1YBc+gxZr0CUtJwxaFJ-WWimJNvA@mail.gmail.com>
 <ZwY68OhKx/5jWeCj@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoCAEXmxaDHAjMwqcpV6JVa91wBbXamQbk-rv1QnYhzGhA@mail.gmail.com>
 <ZwdK1UYdLn/zFMHy@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoB-bxBqBEt9zbUQKTTUag1_NRPeT_5aGXC_j+z0+T3Hkw@mail.gmail.com>
 <ZwfSXS/3v73388jh@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoCoyNBCahi6efsjG9Lr_dGsVin1m6Y6tj4dTE6UnTAcbw@mail.gmail.com>
 <Zwkk9Z6rzNxArr1p@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoB31apHCzZ-45rDtB56qQm+TBwJnB48JjBvuqR5xviAEQ@mail.gmail.com>
 <CAD21AoD+opO0SaDSFvz+n+wAxc-kUrpF5Wtxbw80Bqpay+rkgw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAD21AoD+opO0SaDSFvz+n+wAxc-kUrpF5Wtxbw80Bqpay+rkgw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Zwy3goasKiSBQeEe%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

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