Re: Add contrib/pg_logicalsnapinspect

Hi,

On Wed, Sep 25, 2024 at 05:04:18PM -0700, Masahiko Sawada wrote:
> I've reviewed v10 patch and here are some comments:

Thanks for looking at it!

> 
>  +static void
>  +ValidateAndRestoreSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
>  +                              const char *path)
> 
> This function and SnapBuildRestore() have duplicate codes. Can we have
> a common function that reads the snapshot data from disk to
> SnapBuildOnDisk, and have both ValidateAndRestoreSnapshotFile() and
> SnapBuildRestore() call it?

Right. I had in mind to remove the duplicate code while proposing the "in core"
functions version of this patch, see [1]. Now that snapbuild_internal.h is there,
I do not see any reason not to remove the duplicate code.

That's done in v11 attached: ValidateAndRestoreSnapshotFile() has been modified
a bit and can now be used in the new module and in SnapBuildRestore().

Bonus points, as compared to v10, it allows to:

1. move the SnapBuildRestoreContents() declaration back from snapbuild_internal.h
to its original location (snapbuild.c)
2. same as above for the SnapBuildOnDiskConstantSize, SnapBuildOnDiskNotChecksummedSize,
SNAPBUILD_MAGIC and SNAPBUILD_VERSION definitions
3. remove the changes in pg_crc32c.h as the extras "PGDLLIMPORT" are not needed
anymore

> ---
> +CREATE FUNCTION pg_get_logical_snapshot_meta(IN in_lsn pg_lsn,
> (snip)
> +CREATE FUNCTION pg_get_logical_snapshot_info(IN in_lsn pg_lsn,
> 
> Is there any reason why both functions take a pg_lsn value as an
> argument? Given that the main usage would be to use these functions
> with pg_ls_logicalsnapdir(), I think it would be easier for users if
> these functions take a filename as a function argument. That way, we
> can use these functions like:
> 
> select info.* from pg_ls_logicalsnapdir(),
> pg_get_logical_snapshot_info(name) as info;

I think that makes sense. It also simplfies the tests and the examples, done
that way in v11.

> 
> If there are use cases where specifying a LSN is easier, I think we
> would have both types.
> 
> ----
> +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",
> +};
> 
> I think that it'd be better to have a dedicated function that returns
> a string representation of the given state by using a switch
> statement. That way, we don't need SNAPBUILD_STATE_INCR and a compiler
> warning would help realize a missing state if a new state is
> introduced in the future.

Yeah, that sounds reasonable. Done in v11: the switch does not handle "default"
so that [-Wswitch] would report a warning in case of enumeration value not
handled in the switch (v11 keeps a remark on top of the SnapBuildState enum
definition though).

> It needs a function call but I believe it won't be a problem in this use case.

Agree.


[1]: https://www.postgresql.org/message-id/ZtGKi5FdW%2Bky%2B0fV%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