My review comments for v8-0001
======
contrib/pg_logicalinspect/pg_logicalinspect.c
1.
+/*
+ * Lookup table for SnapBuildState.
+ */
+
+#define SNAPBUILD_STATE_INCR 1
+
+static const char *const SnapBuildStateDesc[] = {
+ [SNAPBUILD_START + SNAPBUILD_STATE_INCR] = "start",
+ [SNAPBUILD_BUILDING_SNAPSHOT + SNAPBUILD_STATE_INCR] = "building",
+ [SNAPBUILD_FULL_SNAPSHOT + SNAPBUILD_STATE_INCR] = "full",
+ [SNAPBUILD_CONSISTENT + SNAPBUILD_STATE_INCR] = "consistent",
+};
+
+/*
nit - the SNAPBUILD_STATE_INCR made this code appear more complicated
than it is. Please take a look at the attachment for an alternative
implementation which includes an explanatory comment. YMMV. Feel free
to ignore it.
======
src/include/replication/snapbuild.h
2.
+ * Please keep SnapBuildStateDesc[] (located in the pg_logicalinspect module)
+ * updated should a change needs to be done in SnapBuildState.
nit - "...should a change needs to be done" -- the word "needs" is
incorrect here.
How about:
"...if a change needs to be made to SnapBuildState."
"...if a change is made to SnapBuildState."
"...if SnapBuildState is changed."
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c
b/contrib/pg_logicalinspect/pg_logicalinspect.c
index 100b82f..2419df1 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -24,19 +24,6 @@ PG_FUNCTION_INFO_V1(pg_get_logical_snapshot_meta);
PG_FUNCTION_INFO_V1(pg_get_logical_snapshot_info);
/*
- * Lookup table for SnapBuildState.
- */
-
-#define SNAPBUILD_STATE_INCR 1
-
-static const char *const SnapBuildStateDesc[] = {
- [SNAPBUILD_START + SNAPBUILD_STATE_INCR] = "start",
- [SNAPBUILD_BUILDING_SNAPSHOT + SNAPBUILD_STATE_INCR] = "building",
- [SNAPBUILD_FULL_SNAPSHOT + SNAPBUILD_STATE_INCR] = "full",
- [SNAPBUILD_CONSISTENT + SNAPBUILD_STATE_INCR] = "consistent",
-};
-
-/*
* 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.
*/
@@ -186,6 +173,16 @@ Datum
pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
{
#define PG_GET_LOGICAL_SNAPSHOT_INFO_COLS 14
+ /*
+ * Lookup table for SnapBuildState. The lookup index is offset by 1
+ * because the consecutive SnapBuildState enum values start at -1.
+ */
+ static const char *const SnapBuildStateDesc[] = {
+ [1 + SNAPBUILD_START] = "start",
+ [1 + SNAPBUILD_BUILDING_SNAPSHOT] = "building",
+ [1 + SNAPBUILD_FULL_SNAPSHOT] = "full",
+ [1 + SNAPBUILD_CONSISTENT] = "consistent",
+ };
SnapBuildOnDisk ondisk;
XLogRecPtr lsn;
HeapTuple tuple;
@@ -209,8 +206,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
memset(nulls, 0, sizeof(nulls));
- values[i++] =
CStringGetTextDatum(SnapBuildStateDesc[ondisk.builder.state +
-
SNAPBUILD_STATE_INCR]);
+ values[i++] =
CStringGetTextDatum(SnapBuildStateDesc[ondisk.builder.state + 1]);
values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
values[i++] = TransactionIdGetDatum(ondisk.builder.xmax);
values[i++] = LSNGetDatum(ondisk.builder.start_decoding_at);
diff --git a/src/include/replication/snapbuild.h
b/src/include/replication/snapbuild.h
index 78df2d1..e844a89 100644
--- a/src/include/replication/snapbuild.h
+++ b/src/include/replication/snapbuild.h
@@ -17,7 +17,7 @@
/*
* Please keep SnapBuildStateDesc[] (located in the pg_logicalinspect module)
- * updated should a change needs to be done in SnapBuildState.
+ * updated if a change needs to be made to SnapBuildState.
*/
typedef enum
{
