Re: Add contrib/pg_logicalsnapinspect

[Peter Smith]

Thanks for the updated patch.

Here are a few more trivial comments for the patch v7-0001.

======

1.
Should the extension descriptions all be identical?

I noticed small variations:

contrib/pg_logicalinspect/Makefile
+PGFILEDESC = "pg_logicalinspect - functions to inspect logical
decoding components"

contrib/pg_logicalinspect/meson.build
+    '--FILEDESC', 'pg_logicalinspect - functions to inspect contents
of logical snapshots',])

contrib/pg_logicalinspect/pg_logicalinspect.control
+comment = 'functions to inspect logical decoding components'

======
.../expected/logical_inspect.out

2
+step s1_get_logical_snapshot_info: SELECT
(pg_get_logical_snapshot_info(f.name::pg_lsn)).state,(pg_get_logical_snapshot_info(f.name::pg_lsn)).catchange_count,array_length((pg_get_logical_snapshot_info(f.name::pg_lsn)).catchange_xip,1),(pg_get_logical_snapshot_info(f.name::pg_lsn)).committed_count,array_length((pg_get_logical_snapshot_info(f.name::pg_lsn)).committed_xip,1)
FROM (SELECT replace(replace(name,'.snap',''),'-','/') AS name FROM
pg_ls_logicalsnapdir()) AS f ORDER BY 2;
+state|catchange_count|array_length|committed_count|array_length
+-----+---------------+------------+---------------+------------
+    2|              0|            |              2|           2
+    2|              2|           2|              0|
+(2 rows)
+

2a.
Would it be better to rearrange those columns so 'committed' stuff
comes before 'catchange' stuff, to make this table order consistent
with the structure/code?

~

2b.
Maybe those 2 'array_length' columns could have aliases to uniquely
identify them?
e.g. 'catchange_array_length' and 'committed_array_length'.

======
contrib/pg_logicalinspect/pg_logicalinspect.c

3.
+/*
+ * Validate the logical snapshot file.
+ */
+static void
+ValidateAndRestoreSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
+    const char *path)

Since the name was updated then should the function comment also be
updated to include something like the SnapBuildRestoreContents
function comment? e.g. "Validate the logical snapshot file, and read
the contents of the serialized snapshot to 'ondisk'."

~~~

pg_get_logical_snapshot_info:

4.
nit - Add/remove some blank lines to help visually associate the array
counts with their arrays.

======
.../specs/logical_inspect.spec

5.
+setup
+{
+    DROP TABLE IF EXISTS tbl1;
+    CREATE TABLE tbl1 (val1 integer, val2 integer);
+ CREATE EXTENSION pg_logicalinspect;
+}
+
+teardown
+{
+    DROP TABLE tbl1;
+    SELECT 'stop' FROM pg_drop_replication_slot('isolation_slot');
+ DROP EXTENSION pg_logicalinspect;
+}

Different indentation for the CREATE/DROP EXTENSION?

======

The attached file shows the whitespace nit (#4)

======
Kind Regards,
Peter Smith.
Fujitsu Australia

diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c 
b/contrib/pg_logicalinspect/pg_logicalinspect.c
index 185f36a..308c653 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -205,8 +205,8 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
        values[i++] = BoolGetDatum(ondisk.builder.in_slot_creation);
        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++] = Int64GetDatum(ondisk.builder.committed.xcnt);
        if (ondisk.builder.committed.xcnt > 0)
        {
                Datum      *arrayelems;
@@ -223,7 +223,6 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
                nulls[i++] = true;
 
        values[i++] = Int64GetDatum(ondisk.builder.catchange.xcnt);
-
        if (ondisk.builder.catchange.xcnt > 0)
        {
                Datum      *arrayelems;