Re: Add contrib/pg_logicalsnapinspect

Hi,

On Wed, Sep 25, 2024 at 10:29 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Wed, Sep 25, 2024 at 04:04:43PM +0200, Peter Eisentraut wrote:
> > Is there a reason for this elaborate error handling:
>
> Thanks for looking at it!
>
> > + fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
> > +
> > + if (fd < 0 && errno == ENOENT)
> > + ereport(ERROR,
> > + errmsg("file \"%s\" does not exist", path));
> > + else if (fd < 0)
> > + ereport(ERROR,
> > + (errcode_for_file_access(),
> > + errmsg("could not open file \"%s\": %m", path)));
> >
> > Couldn't you just use the second branch for all errno's?
>
> Yeah, I think it comes from copying/pasting from SnapBuildRestore() too "quickly".
> v10 attached uses the second branch only.
>

I've reviewed v10 patch and here are some comments:

+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?

---
+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;

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. It needs a function call but I believe it
won't be a problem in this use case.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com