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
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c
b/contrib/pg_logicalinspect/pg_logicalinspect.c
index dc9041a..2111202 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -1,13 +1,17 @@
/*-------------------------------------------------------------------------
*
* pg_logicalinspect.c
- * Functions to inspect contents of PostgreSQL logical snapshots
+ * Functions to inspect contents of PostgreSQL logical snapshots
*
* Copyright (c) 2024, PostgreSQL Global Development Group
*
* IDENTIFICATION
- * contrib/pg_logicalinspect/pg_logicalinspect.c
+ * contrib/pg_logicalinspect/pg_logicalinspect.c
*
+ *
+ * NOTES
+ * For any code change or issue fix here, it is highly recommended
to
+ * give a thought about doing the same in SnapBuildRestore() as
well.
*-------------------------------------------------------------------------
*/
#include "postgres.h"
@@ -23,18 +27,12 @@ PG_FUNCTION_INFO_V1(pg_get_logical_snapshot_meta);
PG_FUNCTION_INFO_V1(pg_get_logical_snapshot_info);
/*
- * 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.
- */
-
-/*
* Validate the logical snapshot file.
*/
static void
ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk, const char *path)
{
int fd;
- Size sz;
pg_crc32c checksum;
MemoryContext context;
@@ -63,7 +61,6 @@ ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
const char *path)
fsync_fname(path, false);
fsync_fname(PG_LOGICAL_SNAPSHOTS_DIR, true);
-
/* read statically sized portion of snapshot */
SnapBuildRestoreContents(fd, (char *) ondisk,
SnapBuildOnDiskConstantSize, path);
@@ -93,7 +90,7 @@ ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
const char *path)
/* restore committed xacts information */
if (ondisk->builder.committed.xcnt > 0)
{
- sz = sizeof(TransactionId) * ondisk->builder.committed.xcnt;
+ Size sz = sizeof(TransactionId) *
ondisk->builder.committed.xcnt;
ondisk->builder.committed.xip =
MemoryContextAllocZero(ondisk->builder.context, sz);
SnapBuildRestoreContents(fd, (char *)
ondisk->builder.committed.xip, sz, path);
COMP_CRC32C(checksum, ondisk->builder.committed.xip, sz);
@@ -102,7 +99,7 @@ ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk
*ondisk, const char *path)
/* restore catalog modifying xacts information */
if (ondisk->builder.catchange.xcnt > 0)
{
- sz = sizeof(TransactionId) * ondisk->builder.catchange.xcnt;
+ Size sz = sizeof(TransactionId) *
ondisk->builder.catchange.xcnt;
ondisk->builder.catchange.xip =
MemoryContextAllocZero(ondisk->builder.context, sz);
SnapBuildRestoreContents(fd, (char *)
ondisk->builder.catchange.xip, sz, path);
COMP_CRC32C(checksum, ondisk->builder.catchange.xip, sz);
@@ -210,12 +207,11 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
if (ondisk.builder.committed.xcnt > 0)
{
Datum *arrayelems;
- int narrayelems;
+ int narrayelems = 0;
arrayelems = (Datum *) palloc(ondisk.builder.committed.xcnt *
sizeof(Datum));
- narrayelems = 0;
- for (narrayelems = 0; narrayelems <
ondisk.builder.committed.xcnt; narrayelems++)
+ for (; narrayelems < ondisk.builder.committed.xcnt;
narrayelems++)
arrayelems[narrayelems] = Int64GetDatum((int64)
ondisk.builder.committed.xip[narrayelems]);
values[i++] =
PointerGetDatum(construct_array_builtin(arrayelems, narrayelems, INT8OID));
@@ -228,12 +224,11 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
if (ondisk.builder.catchange.xcnt > 0)
{
Datum *arrayelems;
- int narrayelems;
+ int narrayelems = 0;
arrayelems = (Datum *) palloc(ondisk.builder.catchange.xcnt *
sizeof(Datum));
- narrayelems = 0;
- for (narrayelems = 0; narrayelems <
ondisk.builder.catchange.xcnt; narrayelems++)
+ for (; narrayelems < ondisk.builder.catchange.xcnt;
narrayelems++)
arrayelems[narrayelems] = Int64GetDatum((int64)
ondisk.builder.catchange.xip[narrayelems]);
values[i++] =
PointerGetDatum(construct_array_builtin(arrayelems, narrayelems, INT8OID));
diff --git a/doc/src/sgml/pglogicalinspect.sgml
b/doc/src/sgml/pglogicalinspect.sgml
index 7767de2..3cc7742 100644
--- a/doc/src/sgml/pglogicalinspect.sgml
+++ b/doc/src/sgml/pglogicalinspect.sgml
@@ -10,7 +10,7 @@
<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
+ allows the inspection of serialized logical snapshots of a running
<productname>PostgreSQL</productname> database cluster, which is useful
for debugging or educational purposes.
</para>
@@ -38,7 +38,7 @@
the <filename>pg_logical/snapshots</filename> directory.
The <replaceable>in_lsn</replaceable> argument can be extracted from the
snapshot file name.
- example:
+ For example:
<screen>
postgres=# SELECT * FROM pg_ls_logicalsnapdir();
-[ RECORD 1 ]+-----------------------
@@ -79,7 +79,7 @@ version | 6
the <filename>pg_logical/snapshots</filename> directory.
The <replaceable>in_lsn</replaceable> argument can be extracted from the
snapshot file name.
- example:
+ For example:
<screen>
postgres=# SELECT * FROM pg_ls_logicalsnapdir();
-[ RECORD 1 ]+-----------------------
