Horton

Re: Add contrib/pg_logicalsnapinspect

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-09-23T02:27:27Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix indentation issue

  2. pg_logicalinspect: Stabilize isolation tests.

  3. Add contrib/pg_logicalinspect.

  4. Move SnapBuild and SnapBuildOnDisk structs to snapshot_internal.h.

Attachments

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