On Fri, Oct 11, 2024 at 6:15 AM Bertrand Drouvot <bertranddrouvot...@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...@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
