Re: Add contrib/pg_logicalsnapinspect

Hi,

On Tue, Oct 08, 2024 at 04:25:29PM +1100, Peter Smith wrote:
> Hi, here are some review comments for patch v11.

Thanks for looking at it!

> ======
> contrib/pg_logicalinspect/specs/logical_inspect.spec
> 
> 1.
> nit - Add some missing spaces after commas (,) in the SQL.

Fine by me, done in v12 attached.

> ======
> doc/src/sgml/pglogicalinspect.sgml
> 
> 2.
> + <note>
> +  <para>
> +   The <filename>pg_logicalinspect</filename> functions are called
> +   using a text argument that can be extracted from the output name of the
> +   <function>pg_ls_logicalsnapdir</function>() function.
> +  </para>
> + </note>
> 
> 2a. wording
> 
> The wording "using a text argument that can be extracted" seems like a
> hangover from the previous implementation; it does not even say what
> that "text argument" means.

That's right (it's mentioned later on (for each function description) that
the argument represents the snapshot file name though).

> Why not just say it is a snapshot
> filename, something like below?
> 
> SUGGESTION:
> The pg_logicalinspect functions are called passing a snapshot filename
> to be inspected. For example, pass a name obtained from the
> pg_ls_logicalsnapdir() function.

Yeah, I like it, but...

> ~
> 
> 2b.  formatting
> 
> nit - In the previous implementation the extraction of the LSN was
> trickier, so this part was worthy of an SGML "NOTE". Now that it is
> just a filename, I don't know if it needs to be a special note
> anymore.

In fact, giving it more thoughts, I think we can just remove this part.
I don't see the extra value anymore and that's something that we may need to
remove depending on what will be added to this module in the future.

I think that having the argument explanation in each function description is
enough, done that way in v12.

> 
> ~~~
> 
> 3.
> +postgres=# SELECT meta.* FROM pg_ls_logicalsnapdir(),
> +pg_get_logical_snapshot_meta(name) AS meta;
> +
> +-[ RECORD 1 ]--------
> +magic    | 1369563137
> +checksum | 1028045905
> +version  | 6
> 
> 3a.
> If you are going to wrap the SQL across multiple lines like this, then
> you should show the psql continuation prompt, so that the example
> looks the same as what the user would see.

I'm not sure about this one. If the user copy/paste the doc as it is then there
is no psql continuation prompt. If the user does not copy/paste the doc then he
might indeed see "something" else (but that's not surprising since he did not
copy/paste). FWIW, there is similar examples in pgstatstatements.sgml.

> ~
> 
> 3b.
> FYI, the output of that can return multiple records,

Yes, as the test in this patch does.

> which is
> b.i) probably not what you intended to demonstrate
> b.ii) not the same as what the example says
> 
> e.g., I got this:
> test_pub=# SELECT meta.* FROM pg_ls_logicalsnapdir(),
> test_pub-# pg_get_logical_snapshot_meta(name) AS meta;
> -[ RECORD 1 ]--------
> magic    | 1369563137
> checksum | 681884630
> version  | 6
> -[ RECORD 2 ]--------
> magic    | 1369563137
> checksum | 2213048308
> version  | 6
> -[ RECORD 3 ]--------
> magic    | 1369563137
> checksum | 3812680762
> version  | 6
> -[ RECORD 4 ]--------
> magic    | 1369563137
> checksum | 3759893001
> version  | 6
>

I don't get the point here. The examples just show another way to use the functions,
the ouput is more "anecdotal" than anything else.
 
> 
> ~~~
> 
> (Also those #3a, #3b comments apply to both examples)
> 
> ======
> src/backend/replication/logical/snapbuild.c
> 
> 4.
> - SnapBuild builder;
> -
> - /* variable amount of TransactionIds follows */
> -} SnapBuildOnDisk;
> -
>  #define SnapBuildOnDiskConstantSize \
>   offsetof(SnapBuildOnDisk, builder)
>  #define SnapBuildOnDiskNotChecksummedSize \
> 
> Is it better to try to keep those "Size" macros defined along with
> wherever the SnapBuildOnDisk is defined? Otherwise, if the structure
> is ever changed, adjusting the macros could be easily overlooked.

I think that the less we put in the snapbuild_internal.h the better. That said,
I think you have a good point so I added a comment around the SnapBuildOnDisk
definition instead in v12.

> 
> ~~~
> 
> 5.
> ValidateAndRestoreSnapshotFile
> 
> nit - See [1] #4 suggestion to declare 'sz' at scope where used. The
> previous reason not to change this (e.g. "mainly inspired from
> SnapBuildRestore") seems less relevant because now most lines of this
> function have already been modified for other reasons.

Right. I think that's a matter of taste and I do prefer to "only" do the
necessary changes that are linked to the feature the patch is implementing.

> ~~~
> 
> 6.
> SnapBuildRestore:
> 
> + if (fd < 0 && errno == ENOENT)
> + return false;
> + else if (fd < 0)
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not open file \"%s\": %m", path)));
> 
> I think this code fragment looked like this before, and you only
> relocated it,

That's right.

> but it still seems a bit awkward to write this way.
> Since so much else has changed, how about also improving this in
> passing, like below:
> 
> if (fd < 0)
> {
>   if (errno == ENOENT)
>     return false;
> 
>   ereport(ERROR,
>     (errcode_for_file_access(),
>     errmsg("could not open file \"%s\": %m", path)));
> }

Same, I do prefer to "only" do the necessary changes that are linked to the
feature the patch is implementing (and why stop here, a similar change could be
made in logical/origin.c too for example).

Regards,

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