Re: Add contrib/pg_logicalsnapinspect

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(dot)pg(at)gmail(dot)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