Horton

Re: Add contrib/pg_logicalsnapinspect

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

View thread
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Peter Smith <smithpb2250@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-11T13:15:33Z
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 Thu, Oct 10, 2024 at 05:38:43PM -0700, Masahiko Sawada wrote:
> On Thu, Oct 10, 2024 at 6:10 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> 
> The patches mostly look good to me. Here are some minor comments:

Thanks for looking at it!

> 
> +       sprintf(path, "%s/%s",
> +                       PG_LOGICAL_SNAPSHOTS_DIR,
> +                       text_to_cstring(filename_t));
> +
> +       /* Validate and restore the snapshot to 'ondisk' */
> +       ValidateAndRestoreSnapshotFile(&ondisk, path,
> CurrentMemoryContext, false);
> +
> +       /* 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");
> +
> I think it would be better to check the result type before reading the
> snapshot file.

Agree, done in v14.

> 
> ---
> +       values[i++] = Int64GetDatum((int64) ondisk.checksum);
> 
> Why is only checksum casted to int64? With that, it can show a
> checksum value as a non-netagive integer but is it really necessary?
> For instance, page_header() function in pageinspect shows a page
> checksum as smallint.

Yeah, pd_checksum in PageHeaderData is uint16 while checksum in SnapBuildOnDisk
is pg_crc32c. The reason why it is casted to int64 is explained in [1], does that
make sense to you?

> ---@@ -0,0 +1,4 @@
> +# Generated subdirectories
> +/log/
> +/results/
> +/tmp_check/
> 
> output_iso and tmp_check_iso should be added.

Yeah, done in v14.

> ---
>  /*
> - * Restore a snapshot into 'builder' if previously one has been stored at the
> - * location indicated by 'lsn'. Returns true if successful, false otherwise.
> + * Validate the logical snapshot file and read its contents to 'ondisk'.
>   */
> -static bool
> -SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
> +bool
> +ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk, const char *path,
> +                              MemoryContext context, bool missing_ok)
> 
> I think it would be better to add some descriptions of the function
> arguments, particularly context and missing_ok.

Thought about it but somehow managed to missed it. Thanks, done in v14.

> The names of all other functions in snapbuild.c have the "SnapBuild"
> prefix. I think it's better to follow it. How about renaming it to
> SnapBuildReadSnapshot(), SnapBuildRestoreSnapshot(), or something
> along those lines?

That makes sense. I opted for SnapBuildRestoreSnapshot() (as it's calling
SnapBuildRestoreContents()). 

[1]: https://www.postgresql.org/message-id/ZvLuhh5pzpIqolkW%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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