Horton

Re: Add contrib/pg_logicalsnapinspect

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, 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-10-13T22:57:22Z
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.

Here are some minor review comments for v15-0002.

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

1.
+pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
+{
+#define PG_GET_LOGICAL_SNAPSHOT_META_COLS 3
+ SnapBuildOnDisk ondisk;
+ HeapTuple tuple;
+ Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
+ bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
+ TupleDesc tupdesc;
+ char path[MAXPGPATH];
+ int i = 0;
+ text    *filename_t = PG_GETARG_TEXT_PP(0);
+
+ sprintf(path, "%s/%s",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ text_to_cstring(filename_t));
+
+ /* Build a tuple descriptor for our result type */
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
+
+ /* Validate and restore the snapshot to 'ondisk' */
+ SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);

The sprintf should be deferred. Could you do it after the ERROR check?

~~~

2.
+pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
+{
+#define PG_GET_LOGICAL_SNAPSHOT_INFO_COLS 14
+ SnapBuildOnDisk ondisk;
+ HeapTuple tuple;
+ Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
+ bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
+ TupleDesc tupdesc;
+ char path[MAXPGPATH];
+ int i = 0;
+ text    *filename_t = PG_GETARG_TEXT_PP(0);
+
+ sprintf(path, "%s/%s",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ text_to_cstring(filename_t));
+
+ /* Build a tuple descriptor for our result type */
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");

Ditto #1. The sprintf should be deferred. Could you do it after the ERROR check?

======
src/backend/replication/logical/snapbuild.c

3.
 /*
- * Restore a snapshot into 'builder' if previously one has been stored at the
- * location indicated by 'lsn'. Returns true if successful, false otherwise.
+ * Restore the logical snapshot file contents to 'ondisk'.
+ *
+ * If 'missing_ok' is true, will not throw an error if the file is not found.
+ * 'context' is the memory context where the catalog modifying/committed xid
+ * will live.
  */
-static bool
-SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
+bool
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+ MemoryContext context, bool missing_ok)

nit - I think it's better to describe parameters in the same order
that they are declared. Also, include a 'path' description, so it is
not the only one omitted.

SUGGESTION:
'path' - snapshot file path.
'context' - memory context where the catalog modifying/committed xid will live.
‘missing_ok’ – when true, don't throw an error if the file is not found.

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