On Fri, Oct 11, 2024 at 11:15 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > 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?
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
commit 483f5a402b6f650d02e8f8668bd50b17680f2805
Author: Masahiko Sawada <sawada.m...@gmail.com>
Date: Fri Oct 11 15:52:49 2024 -0700
minor changes
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
b/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
index c773f6e458..8f7f947cbb 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
+++ b/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
@@ -31,9 +31,9 @@ CREATE FUNCTION pg_get_logical_snapshot_info(IN filename text,
OUT in_slot_creation boolean,
OUT last_serialized_snapshot pg_lsn,
OUT next_phase_at xid,
- OUT committed_count int8,
+ OUT committed_count int4,
OUT committed_xip xid[],
- OUT catchange_count int8,
+ OUT catchange_count int4,
OUT catchange_xip xid[]
)
AS 'MODULE_PATHNAME', 'pg_get_logical_snapshot_info'
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c
b/contrib/pg_logicalinspect/pg_logicalinspect.c
index fabfe2a10c..790c64d6fa 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -123,7 +123,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
values[i++] = LSNGetDatum(ondisk.builder.last_serialized_snapshot);
values[i++] = TransactionIdGetDatum(ondisk.builder.next_phase_at);
- values[i++] = Int64GetDatum(ondisk.builder.committed.xcnt);
+ values[i++] = UInt32GetDatum(ondisk.builder.committed.xcnt);
if (ondisk.builder.committed.xcnt > 0)
{
Datum *arrayelems;
@@ -140,7 +140,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
else
nulls[i++] = true;
- values[i++] = Int64GetDatum(ondisk.builder.catchange.xcnt);
+ values[i++] = UInt32GetDatum(ondisk.builder.catchange.xcnt);
if (ondisk.builder.catchange.xcnt > 0)
{
Datum *arrayelems;
diff --git a/doc/src/sgml/pglogicalinspect.sgml
b/doc/src/sgml/pglogicalinspect.sgml
index e0fac997b6..4b111f9611 100644
--- a/doc/src/sgml/pglogicalinspect.sgml
+++ b/doc/src/sgml/pglogicalinspect.sgml
@@ -22,7 +22,7 @@
</para>
<sect2 id="pglogicalinspect-funcs">
- <title>General Functions</title>
+ <title>Functions</title>
<variablelist>
<varlistentry id="pglogicalinspect-funcs-pg-get-logical-snapshot-meta">
v15-0001-Move-SnapBuild-and-SnapBuildOnDisk-structs-to-sn.patch
Description: Binary data
v15-0002-Add-contrib-pg_logicalinspect.patch
Description: Binary data
