Re: Add contrib/pg_logicalsnapinspect

HI, here are some mostly minor review comments for the patch v5-0001.

======
Commit message

1.
Do you think you should also name the new functions here?

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

2.
Regarding the question about static function declarations:

Shveta wrote: I was somehow under the impression that this is the way
in the postgres i.e. not add redundant declarations. Will be good to
know what others think on this.

FWIW, my understanding is the convention is just to be consistent with
whatever the module currently does. If it declares static functions,
then declare them all (redundant or not). If it doesn't declare static
functions, then don't add one. But, in the current case, since this is
a new module, I guess it is entirely up to you whatever you want to
do.

~~~

3.
+/*
+ * NOTE: For any code change or issue fix here, it is highly recommended to
+ * give a thought about doing the same in SnapBuildRestore() as well.
+ */
+

nit - I think this NOTE should be part of this module's header
comment. (e.g. like the tablesync.c NOTES)

~~~

ValidateSnapshotFile:

4.
+ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk, const char *path)
+{
+ int fd;
+ Size sz;

nit - The 'sz' is overwritten a few times. I thnk declaring it at each
scope where used would be tidier.

~~~

5.
+ fsync_fname(path, false);
+ fsync_fname(PG_LOGICAL_SNAPSHOTS_DIR, true);
+
+

nit - remove some excessive blank lines

~~~

6.
+ /* read statically sized portion of snapshot */
+ SnapBuildRestoreContents(fd, (char *) ondisk,
SnapBuildOnDiskConstantSize, path);

Should that say "fixed size portion"?

~~~

pg_get_logical_snapshot_info:

7.
+ if (ondisk.builder.committed.xcnt > 0)
+ {
+ Datum *arrayelems;
+ int narrayelems;
+
+ arrayelems = (Datum *) palloc(ondisk.builder.committed.xcnt * sizeof(Datum));
+ narrayelems = 0;
+
+ for (narrayelems = 0; narrayelems < ondisk.builder.committed.xcnt;
narrayelems++)
+ arrayelems[narrayelems] = Int64GetDatum((int64)
ondisk.builder.committed.xip[narrayelems]);

nit - Why the double assignment of narrayelems = 0? It is simpler to
assign at the declaration and then remove both others.

~~~

8.
+ if (ondisk.builder.catchange.xcnt > 0)
+ {
+ Datum *arrayelems;
+ int narrayelems;
+
+ arrayelems = (Datum *) palloc(ondisk.builder.catchange.xcnt * sizeof(Datum));
+ narrayelems = 0;
+
+ for (narrayelems = 0; narrayelems < ondisk.builder.catchange.xcnt;
narrayelems++)
+ arrayelems[narrayelems] = Int64GetDatum((int64)
ondisk.builder.catchange.xip[narrayelems]);

nit - ditto previous comment

======
doc/src/sgml/pglogicalinspect.sgml

9.
+ <para>
+ The <filename>pg_logicalinspect</filename> module provides SQL functions
+ that allow you to inspect the contents of logical decoding components. It
+ allows to inspect serialized logical snapshots of a running
+ <productname>PostgreSQL</productname> database cluster, which is useful
+ for debugging or educational purposes.
+ </para>

nit - /It allows to inspect/It allows the inspection of/

~~~

10.
+ example:

nit - /example:/For example:/ (this is in a couple of places)

======
src/include/replication/snapbuild_internal.h

11.
+#ifndef INTERNAL_SNAPBUILD_H
+#define INTERNAL_SNAPBUILD_H

Shouldn't these be SNAPBUILD_INTERNAL_H to match the filename?

~~~

12.
The contents of the snapbuild.c that got moved into
snapbuild_internal.h also got shuffled around a bit.

e.g. originally the typedef struct SnapBuildOnDisk:

+/*
+ * We store current state of struct SnapBuild on disk in the following manner:
+ *
+ * struct SnapBuildOnDisk;
+ * TransactionId * committed.xcnt; (*not xcnt_space*)
+ * TransactionId * catchange.xcnt;
+ *
+ */
+typedef struct SnapBuildOnDisk

was directly beneath the comment:
-/* -----------------------------------
- * Snapshot serialization support
- * -----------------------------------
- */
-

The macros were also defined immediately after the SnapBuildOnDisk
fields they referred to.

Wasn't that original ordering better than how it is now ordered in
snapshot_internal.h?

======

Please also see the attachment, which implements some of those nits
mentioned above.

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