Re: Add contrib/pg_logicalsnapinspect

Hi, here are some review comments for patch v11.

======
contrib/pg_logicalinspect/specs/logical_inspect.spec

1.
nit - Add some missing spaces after commas (,) in the SQL.
nit - Also update the expected results accordingly

======
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. 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.

~

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.

~~~

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.

~

3b.
FYI, the output of that can return multiple records, 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

~~~

(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.

~~~

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.

~~~

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, 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)));
}

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

Kind Regards,
Peter Smith.
Fujitsu Australia