Horton

Re: Add contrib/pg_logicalsnapinspect

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

View thread
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@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-14T06:23:06Z
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

Hi,

On Mon, Oct 14, 2024 at 09:57:22AM +1100, Peter Smith wrote:
> 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?

I think that makes sense, done in v16 attached.

> ======
> 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.

Done in v16.

> Also, include a 'path' description, so it is
> not the only one omitted.

I don't think that's worth it as self explanatory IMHO.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com