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
diff --git a/contrib/pg_logicalinspect/expected/logical_inspect.out
b/contrib/pg_logicalinspect/expected/logical_inspect.out
index 219afd6..71dea4a 100644
--- a/contrib/pg_logicalinspect/expected/logical_inspect.out
+++ b/contrib/pg_logicalinspect/expected/logical_inspect.out
@@ -37,7 +37,7 @@ table public.tbl1: INSERT: val1[integer]:1 val2[integer]:null
COMMIT
(3 rows)
-step s1_get_logical_snapshot_info: SELECT
info.state,info.catchange_count,array_length(info.catchange_xip,1) AS
catchange_array_length,info.committed_count,array_length(info.committed_xip,1)
AS committed_array_length FROM pg_ls_logicalsnapdir(),
pg_get_logical_snapshot_info(name) AS info ORDER BY 2;
+step s1_get_logical_snapshot_info: SELECT info.state, info.catchange_count,
array_length(info.catchange_xip, 1) AS catchange_array_length,
info.committed_count, array_length(info.committed_xip, 1) AS
committed_array_length FROM pg_ls_logicalsnapdir(),
pg_get_logical_snapshot_info(name) AS info ORDER BY 2;
state
|catchange_count|catchange_array_length|committed_count|committed_array_length
----------+---------------+----------------------+---------------+----------------------
consistent| 0| | 2|
2
diff --git a/contrib/pg_logicalinspect/specs/logical_inspect.spec
b/contrib/pg_logicalinspect/specs/logical_inspect.spec
index 7dc3cd7..0a4d268 100644
--- a/contrib/pg_logicalinspect/specs/logical_inspect.spec
+++ b/contrib/pg_logicalinspect/specs/logical_inspect.spec
@@ -29,6 +29,6 @@ setup { SET synchronous_commit=on; }
step "s1_checkpoint" { CHECKPOINT; }
step "s1_get_changes" { SELECT data FROM
pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts',
'1', 'include-xids', '0'); }
step "s1_get_logical_snapshot_meta" { SELECT COUNT(meta.*) from
pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;}
-step "s1_get_logical_snapshot_info" { SELECT
info.state,info.catchange_count,array_length(info.catchange_xip,1) AS
catchange_array_length,info.committed_count,array_length(info.committed_xip,1)
AS committed_array_length FROM pg_ls_logicalsnapdir(),
pg_get_logical_snapshot_info(name) AS info ORDER BY 2; }
+step "s1_get_logical_snapshot_info" { SELECT info.state, info.catchange_count,
array_length(info.catchange_xip, 1) AS catchange_array_length,
info.committed_count, array_length(info.committed_xip, 1) AS
committed_array_length FROM pg_ls_logicalsnapdir(),
pg_get_logical_snapshot_info(name) AS info ORDER BY 2; }
permutation "s0_init" "s0_begin" "s0_savepoint" "s0_truncate" "s1_checkpoint"
"s1_get_changes" "s0_commit" "s0_begin" "s0_insert" "s1_checkpoint"
"s1_get_changes" "s0_commit" "s1_get_changes" "s1_get_logical_snapshot_info"
"s1_get_logical_snapshot_meta"
diff --git a/src/backend/replication/logical/snapbuild.c
b/src/backend/replication/logical/snapbuild.c
index 7a3b963..4b24718 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1691,7 +1691,6 @@ ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk,
const char *path, int fd
MemoryContext
context)
{
pg_crc32c checksum;
- Size sz;
/* ----
* Make sure the snapshot had been stored safely to disk, that's
normally
@@ -1731,7 +1730,7 @@ ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk,
const char *path, int fd
/* restore committed xacts information */
if (ondisk->builder.committed.xcnt > 0)
{
- sz = sizeof(TransactionId) * ondisk->builder.committed.xcnt;
+ Size sz = sizeof(TransactionId) *
ondisk->builder.committed.xcnt;
ondisk->builder.committed.xip = MemoryContextAllocZero(context,
sz);
SnapBuildRestoreContents(fd, (char *)
ondisk->builder.committed.xip, sz, path);
COMP_CRC32C(checksum, ondisk->builder.committed.xip, sz);
@@ -1740,7 +1739,7 @@ ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk,
const char *path, int fd
/* restore catalog modifying xacts information */
if (ondisk->builder.catchange.xcnt > 0)
{
- sz = sizeof(TransactionId) * ondisk->builder.catchange.xcnt;
+ Size sz = sizeof(TransactionId) *
ondisk->builder.catchange.xcnt;
ondisk->builder.catchange.xip = MemoryContextAllocZero(context,
sz);
SnapBuildRestoreContents(fd, (char *)
ondisk->builder.catchange.xip, sz, path);
COMP_CRC32C(checksum, ondisk->builder.catchange.xip, sz);
@@ -1782,13 +1781,15 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
- if (fd < 0 && errno == ENOENT)
- return false;
- else if (fd < 0)
+ if (fd < 0)
+ {
+ if (errno == ENOENT)
+ return false;
+
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m",
path)));
-
+ }
/* validate and restore the snapshot to 'ondisk' */
ValidateAndRestoreSnapshotFile(&ondisk, path, fd, builder->context);
