Subject: Re: Add contrib/pg_logicalsnapinspect

From: Masahiko Sawada <[email-masked]>
Date: Tue, 8 Oct 2024 10:52:11 -0700

On Tue, Oct 8, 2024 at 9:25 AM Bertrand Drouvot
<[email-masked]> wrote:
>
> 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).
>

Thank you for updating the patch! I have some comments on v12 patch:

---
+       if (ondisk.builder.committed.xcnt > 0)
+       {
+               Datum      *arrayelems;
+               int                     narrayelems = 0;
+
+               arrayelems = (Datum *)
palloc(ondisk.builder.committed.xcnt * sizeof(Datum));
+
+               for (; narrayelems < ondisk.builder.committed.xcnt;
narrayelems++)
+                       arrayelems[narrayelems] =
Int64GetDatum((int64) ondisk.builder.committed.xip[narrayelems]);
+
+               values[i++] =
PointerGetDatum(construct_array_builtin(arrayelems, narrayelems,
INT8OID));
+       }

Since committed_xip and catchange_xip are xid[], we should use
TransactionIdGetDatum() and XIDOID instead.

I think that it would be cleaner if we pass
ondisk.builder.committed.xcnt instead of construct_array_builtin to
construct_array_buildin(). That is, we can rewrite it as follows:

for (int j = 0; j < ondisk.builder.committed.xcnt; j++)
    arrayelems[j] = TransactionIdGetDatum(ondisk.builder.committed.xip[j]);

values[i++] = PointerGetDatum(construct_array_builtin(arrayelems,
ondisk.builder.committed.xcnt, XIDOID));

---
+# Test the pg_logicalinspect functions: that needs some permutation to
+# ensure that we are creating multiple logical snapshots and that one of them
+# contains ongoing catalogs changes.

If we use prepared transactions modifying catalog changes, can we
write the normal (i.e. not isolation check) tests? It would be easier
to write and add tests.

---
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/

If we need to use the isolation tests (see above comment), we need to
add both output_iso and tmp_check_iso as well.

---
+       tuple = heap_form_tuple(tupdesc, values, nulls);
+
+       MemoryContextReset(context);
+
+       PG_RETURN_DATUM(HeapTupleGetDatum(tuple));

I think we don't necessarily need to reset the memory context here.
Rather, I think we can just pass CurrentMemoryContext to
ValidateAndRestoreSnapshotFile() instead of passing the separate new
memory context.

---
+       fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
+
+       if (fd < 0)
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not open file \"%s\":
%m", path)));
+
+       context = AllocSetContextCreate(CurrentMemoryContext,
+
 "logicalsnapshot inspect context",
+
 ALLOCSET_DEFAULT_SIZES);
+
+       /* Validate and restore the snapshot to 'ondisk' */
+       ValidateAndRestoreSnapshotFile(&ondisk, path, fd, context);

It's a bit odd to me that this function opens a snapshot file and
passes both the file descriptor and file path. The file path is used
mostly only for error reporting in ValidateAndRestoreSnapshotFile(). I
guess it would be cleaner if we pass the file path to
ValidateAndRestoreSnapshotFile() which opens and validates the
snapshot file. Since SnapBuildRestore() wants to get false if the
specified file doesn't exist, we can also add missing_ok argument to
ValidateAndRestoreSnapshotFile(). That is, the function will be like:

void
ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk, const char
*path, MemoryContext context, bool missing_ok)
{
:
    fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);

    if (fd < 0)
    {
        if (missing_ok && errno == ENOENT)
            return false;
        else
            ereport(ERROR,
                (errcode_for_file_access(),
                 errmsg("could not open file \"%s\": %m", path)));
    }
:

Regards,

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

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 9 Oct 2024 08:12:32 +0000

Hi,

> TransactionIdGetDatum() and XIDOID instead.

I ended up using INT8OID because XIDOID is not part of the switch in
construct_array_builtin() and so leads to:

"
ERROR:  type 28 not supported by construct_array_builtin()
"

One option could be (did not test it) to add this switch in construct_array_builtin():

+               case XIDOID:
+                       elmlen = sizeof(TransactionId);
+                       elmbyval = true;
+                       elmalign = TYPALIGN_INT;
+                       break;

I think that could make sense and would probably need a dedicated patch for that,
thoughts?

> ondisk.builder.committed.xcnt, XIDOID));

Fine by me. Will do that in v13 with TransactionIdGetDatum/XIDOID or Int64GetDatum/INT8OID
once we decide what to do with the above remark linked to construct_array_builtin().

> write the normal (i.e. not isolation check) tests? It would be easier

Not sure about this one. I think that the test is simple enough and mainly inspired
by what can be found in the test_decoding module.

We could still add "normal" (REGRESS) tests in the future should we add features
to the pg_logicalinspect module that would require new tests.

For example, test_decoding is using both kind of tests, what do you think? 

> ValidateAndRestoreSnapshotFile() instead of passing the separate new

Yeah, we should be in a short-lived memory context here (ExprContext or such),
so that's fine by me (will do in v13).

> mostly only for error reporting in ValidateAndRestoreSnapshotFile().

Right.

> I guess it would be cleaner if we pass the file path to

Yeah, it makes sense to move the OpenTransientFile() call in
ValidateAndRestoreSnapshotFile(), will do in v13.

Regards,

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

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 9 Oct 2024 10:21:31 -0700

> ERROR:  type 28 not supported by construct_array_builtin()

Thank you for pointing it out.

> I think that could make sense and would probably need a dedicated patch for that,

Or can we use construct_array() instead?

> For example, test_decoding is using both kind of tests, what do you think?

Fair point. I agree with you.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 10 Oct 2024 03:32:37 +0000

Hi,

> Or can we use construct_array() instead?

I had a closer look to d746021de1 (which introduced construct_array_builtin())
and the hackers thread that lead to it [1].

IIUC, the idea was to:

1. centralize the hardcoded knowledge that were in the calls to construct_array()
and deconstruct_array() for built-in types
2. notational simplification and bug-proofing

As XIDOID is a built-in type, I think that it would make sense to add it in 
deconstruct_array_builtin()/construct_array_builtin().

I think the reason XIDOID has not been added in d746021de1 is that there were no
use case at that time (means no existing calls to construct_array()/deconstruct_array()
with hardcoded XIDOID related arguments).

One could say that we would just add 2 calls to construct_array() in the pg_logicalinspect
module but, for example, d746021de1 also took care of CSTRINGOID that had a single
call at that time:

$ git show d746021de1 | grep deconstruct_array_builtin | grep -c CSTRINGOID
1
$ git show d746021de1 | grep construct_array_builtin | grep -v deconstruct_array_builtin | grep -c CSTRINGOID
1

So I think that having construct_array_builtin()/deconstruct_array_builtin()
taking care of XIDOID is the way to go. If that makes sense to you then I'll
submit a dedicated patch for it, thoughts?

[1]: https://www.postgresql.org/message-id/flat/2914356f-9e5f-8c59-2995-5997fc48bcba%40enterprisedb.com

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 10 Oct 2024 00:05:10 -0700

> submit a dedicated patch for it, thoughts?

Your explanation makes sense to me. I think it can be included in the
main pg_logicalinspect patch as this change is a part of it.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 10 Oct 2024 13:10:53 +0000

Hi,

On Thu, Oct 10, 2024 at 12:05:10AM -0700, Masahiko Sawada wrote:
> Your explanation makes sense to me.

Thanks for sharing your thoughts.

> I think it can be included in the main pg_logicalinspect patch as this change
> is a part of it.

Okay, let's keep the discussion here. Please find attached v13 that takes care
of your previous remarks and Peter's one ([1]).

FYI, v13 is splitted into 2 sub-patches (0001 for the discussion related to
XIDOID and construct_array_builtin() and 0002 for the module itself).

FWIW, the elmbyval and elmalign values that are added in 0001 have been deduced
 from:

postgres=# select typbyval, typalign from pg_type where typname = 'xid';
 typbyval | typalign
----------+----------
 t        | i
(1 row)

[1]: https://www.postgresql.org/message-id/ZwY5vBI%2BR8Ky7yM5%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

>From 6e43fffe9e2995c0183809550aadef505bf4693c Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Thu, 10 Oct 2024 04:09:53 +0000
Subject: [PATCH v13 1/2] Add XIDOID in [de]construct_array_builtin()

Using construct_array_builtin() for XIDOID is a new use case that is coming with
a new module (not added in the code tree yet).

d746021de1 (which introduced construct_array_builtin()) did not take care of
XIDOID because there were no use case at that time. Now that there is one, let's
add XIDOID.
---
 src/backend/utils/adt/arrayfuncs.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 100.0% src/backend/utils/adt/

diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index e5c7e57a5d..8687fae359 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -3447,6 +3447,12 @@ construct_array_builtin(Datum *elems, int nelems, Oid elmtype)
 			elmalign = TYPALIGN_SHORT;
 			break;
 
+		case XIDOID:
+			elmlen = sizeof(TransactionId);
+			elmbyval = true;
+			elmalign = TYPALIGN_INT;
+			break;
+
 		default:
 			elog(ERROR, "type %u not supported by construct_array_builtin()", elmtype);
 			/* keep compiler quiet */
@@ -3734,6 +3740,12 @@ deconstruct_array_builtin(ArrayType *array,
 			elmalign = TYPALIGN_SHORT;
 			break;
 
+		case XIDOID:
+			elmlen = sizeof(TransactionId);
+			elmbyval = true;
+			elmalign = TYPALIGN_INT;
+			break;
+
 		default:
 			elog(ERROR, "type %u not supported by deconstruct_array_builtin()", elmtype);
 			/* keep compiler quiet */
-- 
2.34.1

>From 16d4bc443371a80a4d2617750cfa8b0f22fd6a89 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Wed, 14 Aug 2024 08:46:05 +0000
Subject: [PATCH v13 2/2] Add contrib/pg_logicalinspect

Provides SQL functions that allow to inspect logical decoding components.

It currently allows to inspect the contents of serialized logical snapshots of
a running database cluster, which is useful for debugging or educational
purposes.
---
 contrib/Makefile                              |   1 +
 contrib/meson.build                           |   1 +
 contrib/pg_logicalinspect/.gitignore          |   4 +
 contrib/pg_logicalinspect/Makefile            |  31 ++
 .../expected/logical_inspect.out              |  52 ++++
 contrib/pg_logicalinspect/logicalinspect.conf |   1 +
 contrib/pg_logicalinspect/meson.build         |  39 +++
 .../pg_logicalinspect--1.0.sql                |  43 +++
 contrib/pg_logicalinspect/pg_logicalinspect.c | 171 +++++++++++
 .../pg_logicalinspect.control                 |   5 +
 .../specs/logical_inspect.spec                |  34 +++
 doc/src/sgml/contrib.sgml                     |   1 +
 doc/src/sgml/filelist.sgml                    |   1 +
 doc/src/sgml/pglogicalinspect.sgml            | 143 ++++++++++
 src/backend/replication/logical/snapbuild.c   | 270 ++++--------------
 src/include/replication/snapbuild.h           |   6 +-
 src/include/replication/snapbuild_internal.h  | 199 +++++++++++++
 17 files changed, 788 insertions(+), 214 deletions(-)
   7.7% contrib/pg_logicalinspect/expected/
   5.3% contrib/pg_logicalinspect/specs/
  26.1% contrib/pg_logicalinspect/
  14.0% doc/src/sgml/
  25.9% src/backend/replication/logical/
  20.6% src/include/replication/

diff --git a/contrib/Makefile b/contrib/Makefile
index abd780f277..952855d9b6 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -32,6 +32,7 @@ SUBDIRS = \
 		passwordcheck	\
 		pg_buffercache	\
 		pg_freespacemap \
+		pg_logicalinspect \
 		pg_prewarm	\
 		pg_stat_statements \
 		pg_surgery	\
diff --git a/contrib/meson.build b/contrib/meson.build
index 14a8906865..159ff41555 100644
--- a/contrib/meson.build
+++ b/contrib/meson.build
@@ -46,6 +46,7 @@ subdir('passwordcheck')
 subdir('pg_buffercache')
 subdir('pgcrypto')
 subdir('pg_freespacemap')
+subdir('pg_logicalinspect')
 subdir('pg_prewarm')
 subdir('pgrowlocks')
 subdir('pg_stat_statements')
diff --git a/contrib/pg_logicalinspect/.gitignore b/contrib/pg_logicalinspect/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/contrib/pg_logicalinspect/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/contrib/pg_logicalinspect/Makefile b/contrib/pg_logicalinspect/Makefile
new file mode 100644
index 0000000000..55124514d4
--- /dev/null
+++ b/contrib/pg_logicalinspect/Makefile
@@ -0,0 +1,31 @@
+# contrib/pg_logicalinspect/Makefile
+
+MODULE_big = pg_logicalinspect
+OBJS = \
+	$(WIN32RES) \
+	pg_logicalinspect.o
+PGFILEDESC = "pg_logicalinspect - functions to inspect logical decoding components"
+
+EXTENSION = pg_logicalinspect
+DATA = pg_logicalinspect--1.0.sql
+
+EXTRA_INSTALL = contrib/test_decoding
+
+ISOLATION = logical_inspect
+
+ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/pg_logicalinspect/logicalinspect.conf
+
+# Disabled because these tests require "wal_level=logical", which
+# some installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/pg_logicalinspect
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/pg_logicalinspect/expected/logical_inspect.out b/contrib/pg_logicalinspect/expected/logical_inspect.out
new file mode 100644
index 0000000000..d95efa4d1e
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/logical_inspect.out
@@ -0,0 +1,52 @@
+Parsed test spec with 2 sessions
+
+starting 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
+step s0_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding');
+?column?
+--------
+init    
+(1 row)
+
+step s0_begin: BEGIN;
+step s0_savepoint: SAVEPOINT sp1;
+step s0_truncate: TRUNCATE tbl1;
+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');
+data
+----
+(0 rows)
+
+step s0_commit: COMMIT;
+step s0_begin: BEGIN;
+step s0_insert: INSERT INTO tbl1 VALUES (1);
+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');
+data                                   
+---------------------------------------
+BEGIN                                  
+table public.tbl1: TRUNCATE: (no-flags)
+COMMIT                                 
+(3 rows)
+
+step s0_commit: COMMIT;
+step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
+data                                                         
+-------------------------------------------------------------
+BEGIN                                                        
+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;
+state     |catchange_count|catchange_array_length|committed_count|committed_array_length
+----------+---------------+----------------------+---------------+----------------------
+consistent|              0|                      |              2|                     2
+consistent|              2|                     2|              0|                      
+(2 rows)
+
+step s1_get_logical_snapshot_meta: SELECT COUNT(meta.*) from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;
+count
+-----
+    2
+(1 row)
+
diff --git a/contrib/pg_logicalinspect/logicalinspect.conf b/contrib/pg_logicalinspect/logicalinspect.conf
new file mode 100644
index 0000000000..e3d257315f
--- /dev/null
+++ b/contrib/pg_logicalinspect/logicalinspect.conf
@@ -0,0 +1 @@
+wal_level = logical
diff --git a/contrib/pg_logicalinspect/meson.build b/contrib/pg_logicalinspect/meson.build
new file mode 100644
index 0000000000..3ec635509b
--- /dev/null
+++ b/contrib/pg_logicalinspect/meson.build
@@ -0,0 +1,39 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+pg_logicalinspect_sources = files('pg_logicalinspect.c')
+
+if host_system == 'windows'
+  pg_logicalinspect_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'pg_logicalinspect',
+    '--FILEDESC', 'pg_logicalinspect - functions to inspect logical decoding components',])
+endif
+
+pg_logicalinspect = shared_module('pg_logicalinspect',
+  pg_logicalinspect_sources,
+  kwargs: contrib_mod_args + {
+      'dependencies': contrib_mod_args['dependencies'],
+  },
+)
+contrib_targets += pg_logicalinspect
+
+install_data(
+  'pg_logicalinspect.control',
+  'pg_logicalinspect--1.0.sql',
+  kwargs: contrib_data_args,
+)
+
+tests += {
+  'name': 'pg_logicalinspect',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'isolation': {
+    'specs': [
+      'logical_inspect',
+    ],
+    'regress_args': [
+      '--temp-config', files('logicalinspect.conf'),
+    ],
+    # see above
+    'runningcheck': false,
+  },
+}
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql b/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
new file mode 100644
index 0000000000..c773f6e458
--- /dev/null
+++ b/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
@@ -0,0 +1,43 @@
+/* contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION pg_logicalinspect" to load this file. \quit
+
+--
+-- pg_get_logical_snapshot_meta()
+--
+CREATE FUNCTION pg_get_logical_snapshot_meta(IN filename text,
+    OUT magic int4,
+    OUT checksum int8,
+    OUT version int4
+)
+AS 'MODULE_PATHNAME', 'pg_get_logical_snapshot_meta'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+REVOKE EXECUTE ON FUNCTION pg_get_logical_snapshot_meta(text) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pg_get_logical_snapshot_meta(text) TO pg_read_server_files;
+
+--
+-- pg_get_logical_snapshot_info()
+--
+CREATE FUNCTION pg_get_logical_snapshot_info(IN filename text,
+    OUT state text,
+    OUT xmin xid,
+    OUT xmax xid,
+    OUT start_decoding_at pg_lsn,
+    OUT two_phase_at pg_lsn,
+    OUT initial_xmin_horizon xid,
+    OUT building_full_snapshot boolean,
+    OUT in_slot_creation boolean,
+    OUT last_serialized_snapshot pg_lsn,
+    OUT next_phase_at xid,
+    OUT committed_count int8,
+    OUT committed_xip xid[],
+    OUT catchange_count int8,
+    OUT catchange_xip xid[]
+)
+AS 'MODULE_PATHNAME', 'pg_get_logical_snapshot_info'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+REVOKE EXECUTE ON FUNCTION pg_get_logical_snapshot_info(text) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pg_get_logical_snapshot_info(text) TO pg_read_server_files;
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
new file mode 100644
index 0000000000..a1c43f58cd
--- /dev/null
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -0,0 +1,171 @@
+/*-------------------------------------------------------------------------
+ *
+ * pg_logicalinspect.c
+ *		  Functions to inspect contents of PostgreSQL logical snapshots
+ *
+ * Copyright (c) 2024, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		  contrib/pg_logicalinspect/pg_logicalinspect.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "funcapi.h"
+#include "replication/snapbuild_internal.h"
+#include "utils/array.h"
+#include "utils/builtins.h"
+#include "utils/pg_lsn.h"
+
+PG_MODULE_MAGIC;
+
+PG_FUNCTION_INFO_V1(pg_get_logical_snapshot_meta);
+PG_FUNCTION_INFO_V1(pg_get_logical_snapshot_info);
+
+/* Return the description of SnapBuildState */
+static const char *
+get_snapbuild_state_desc(SnapBuildState state)
+{
+	const char *stateDesc = "unknown state";
+
+	switch (state)
+	{
+		case SNAPBUILD_START:
+			stateDesc = "start";
+			break;
+		case SNAPBUILD_BUILDING_SNAPSHOT:
+			stateDesc = "building";
+			break;
+		case SNAPBUILD_FULL_SNAPSHOT:
+			stateDesc = "full";
+			break;
+		case SNAPBUILD_CONSISTENT:
+			stateDesc = "consistent";
+			break;
+	}
+
+	return stateDesc;
+}
+
+/*
+ * Retrieve the logical snapshot file metadata.
+ */
+Datum
+pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
+{
+#define PG_GET_LOGICAL_SNAPSHOT_META_COLS 3
+	SnapBuildOnDisk ondisk;
+	HeapTuple	tuple;
+	Datum		values[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
+	bool		nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
+	TupleDesc	tupdesc;
+	char		path[MAXPGPATH];
+	int			i = 0;
+	text	   *filename_t = PG_GETARG_TEXT_PP(0);
+
+	sprintf(path, "%s/%s",
+			PG_LOGICAL_SNAPSHOTS_DIR,
+			text_to_cstring(filename_t));
+
+	/* Validate and restore the snapshot to 'ondisk' */
+	ValidateAndRestoreSnapshotFile(&ondisk, path, CurrentMemoryContext, false);
+
+	/* Build a tuple descriptor for our result type */
+	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+		elog(ERROR, "return type must be a row type");
+
+	memset(nulls, 0, sizeof(nulls));
+
+	values[i++] = UInt32GetDatum(ondisk.magic);
+	values[i++] = Int64GetDatum((int64) ondisk.checksum);
+	values[i++] = UInt32GetDatum(ondisk.version);
+
+	Assert(i == PG_GET_LOGICAL_SNAPSHOT_META_COLS);
+
+	tuple = heap_form_tuple(tupdesc, values, nulls);
+
+	PG_RETURN_DATUM(HeapTupleGetDatum(tuple));
+
+#undef PG_GET_LOGICAL_SNAPSHOT_META_COLS
+}
+
+Datum
+pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
+{
+#define PG_GET_LOGICAL_SNAPSHOT_INFO_COLS 14
+	SnapBuildOnDisk ondisk;
+	HeapTuple	tuple;
+	Datum		values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS];
+	bool		nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS];
+	TupleDesc	tupdesc;
+	char		path[MAXPGPATH];
+	int			i = 0;
+	text	   *filename_t = PG_GETARG_TEXT_PP(0);
+
+	sprintf(path, "%s/%s",
+			PG_LOGICAL_SNAPSHOTS_DIR,
+			text_to_cstring(filename_t));
+
+	/* Validate and restore the snapshot to 'ondisk' */
+	ValidateAndRestoreSnapshotFile(&ondisk, path, CurrentMemoryContext, false);
+
+	/* Build a tuple descriptor for our result type */
+	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+		elog(ERROR, "return type must be a row type");
+
+	memset(nulls, 0, sizeof(nulls));
+
+	values[i++] = CStringGetTextDatum(get_snapbuild_state_desc(ondisk.builder.state));
+	values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
+	values[i++] = TransactionIdGetDatum(ondisk.builder.xmax);
+	values[i++] = LSNGetDatum(ondisk.builder.start_decoding_at);
+	values[i++] = LSNGetDatum(ondisk.builder.two_phase_at);
+	values[i++] = TransactionIdGetDatum(ondisk.builder.initial_xmin_horizon);
+	values[i++] = BoolGetDatum(ondisk.builder.building_full_snapshot);
+	values[i++] = BoolGetDatum(ondisk.builder.in_slot_creation);
+	values[i++] = LSNGetDatum(ondisk.builder.last_serialized_snapshot);
+	values[i++] = TransactionIdGetDatum(ondisk.builder.next_phase_at);
+
+	values[i++] = Int64GetDatum(ondisk.builder.committed.xcnt);
+	if (ondisk.builder.committed.xcnt > 0)
+	{
+		Datum	   *arrayelems;
+
+		arrayelems = (Datum *) palloc(ondisk.builder.committed.xcnt * sizeof(Datum));
+
+		for (int j = 0; j < ondisk.builder.committed.xcnt; j++)
+			arrayelems[j] = TransactionIdGetDatum(ondisk.builder.committed.xip[j]);
+
+		values[i++] = PointerGetDatum(construct_array_builtin(arrayelems,
+															  ondisk.builder.committed.xcnt,
+															  XIDOID));
+	}
+	else
+		nulls[i++] = true;
+
+	values[i++] = Int64GetDatum(ondisk.builder.catchange.xcnt);
+	if (ondisk.builder.catchange.xcnt > 0)
+	{
+		Datum	   *arrayelems;
+
+		arrayelems = (Datum *) palloc(ondisk.builder.catchange.xcnt * sizeof(Datum));
+
+		for (int j = 0; j < ondisk.builder.catchange.xcnt; j++)
+			arrayelems[j] = TransactionIdGetDatum(ondisk.builder.catchange.xip[j]);
+
+		values[i++] = PointerGetDatum(construct_array_builtin(arrayelems,
+															  ondisk.builder.catchange.xcnt,
+															  XIDOID));
+	}
+	else
+		nulls[i++] = true;
+
+	Assert(i == PG_GET_LOGICAL_SNAPSHOT_INFO_COLS);
+
+	tuple = heap_form_tuple(tupdesc, values, nulls);
+
+	PG_RETURN_DATUM(HeapTupleGetDatum(tuple));
+
+#undef PG_GET_LOGICAL_SNAPSHOT_INFO_COLS
+}
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.control b/contrib/pg_logicalinspect/pg_logicalinspect.control
new file mode 100644
index 0000000000..b4a70e57ba
--- /dev/null
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.control
@@ -0,0 +1,5 @@
+# pg_logicalinspect extension
+comment = 'functions to inspect logical decoding components'
+default_version = '1.0'
+module_pathname = '$libdir/pg_logicalinspect'
+relocatable = true
diff --git a/contrib/pg_logicalinspect/specs/logical_inspect.spec b/contrib/pg_logicalinspect/specs/logical_inspect.spec
new file mode 100644
index 0000000000..9851a6c18e
--- /dev/null
+++ b/contrib/pg_logicalinspect/specs/logical_inspect.spec
@@ -0,0 +1,34 @@
+# Test the pg_logicalinspect functions: that needs some permutation to
+# ensure that we are creating multiple logical snapshots and that one of them
+# contains ongoing catalogs changes.
+setup
+{
+    DROP TABLE IF EXISTS tbl1;
+    CREATE TABLE tbl1 (val1 integer, val2 integer);
+    CREATE EXTENSION pg_logicalinspect;
+}
+
+teardown
+{
+    DROP TABLE tbl1;
+    SELECT 'stop' FROM pg_drop_replication_slot('isolation_slot');
+    DROP EXTENSION pg_logicalinspect;
+}
+
+session "s0"
+setup { SET synchronous_commit=on; }
+step "s0_init" { SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding'); }
+step "s0_begin" { BEGIN; }
+step "s0_savepoint" { SAVEPOINT sp1; }
+step "s0_truncate" { TRUNCATE tbl1; }
+step "s0_insert" { INSERT INTO tbl1 VALUES (1); }
+step "s0_commit" { COMMIT; }
+
+session "s1"
+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; }
+
+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/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index 44639a8dca..7c381949a5 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -154,6 +154,7 @@ CREATE EXTENSION <replaceable>extension_name</replaceable>;
  &pgbuffercache;
  &pgcrypto;
  &pgfreespacemap;
+ &pglogicalinspect;
  &pgprewarm;
  &pgrowlocks;
  &pgstatstatements;
diff --git a/doc/src/sgml/filelist.sgml b/doc/src/sgml/filelist.sgml
index a7ff5f8264..66e6dccd4c 100644
--- a/doc/src/sgml/filelist.sgml
+++ b/doc/src/sgml/filelist.sgml
@@ -143,6 +143,7 @@
 <!ENTITY pgbuffercache   SYSTEM "pgbuffercache.sgml">
 <!ENTITY pgcrypto        SYSTEM "pgcrypto.sgml">
 <!ENTITY pgfreespacemap  SYSTEM "pgfreespacemap.sgml">
+<!ENTITY pglogicalinspect  SYSTEM "pglogicalinspect.sgml">
 <!ENTITY pgprewarm       SYSTEM "pgprewarm.sgml">
 <!ENTITY pgrowlocks      SYSTEM "pgrowlocks.sgml">
 <!ENTITY pgstatstatements SYSTEM "pgstatstatements.sgml">
diff --git a/doc/src/sgml/pglogicalinspect.sgml b/doc/src/sgml/pglogicalinspect.sgml
new file mode 100644
index 0000000000..e0fac997b6
--- /dev/null
+++ b/doc/src/sgml/pglogicalinspect.sgml
@@ -0,0 +1,143 @@
+<!-- doc/src/sgml/pglogicalinspect.sgml -->
+
+<sect1 id="pglogicalinspect" xreflabel="pg_logicalinspect">
+ <title>pg_logicalinspect &mdash; logical decoding components inspection</title>
+
+ <indexterm zone="pglogicalinspect">
+  <primary>pg_logicalinspect</primary>
+ </indexterm>
+
+ <para>
+  The <filename>pg_logicalinspect</filename> module provides SQL functions
+  that allow you to inspect the contents of logical decoding components. It
+  allows the inspection of serialized logical snapshots of a running
+  <productname>PostgreSQL</productname> database cluster, which is useful
+  for debugging or educational purposes.
+ </para>
+
+ <para>
+  By default, use of these functions is restricted to superusers and members of
+  the <literal>pg_read_server_files</literal> role. Access may be granted by
+  superusers to others using <command>GRANT</command>.
+ </para>
+
+ <sect2 id="pglogicalinspect-funcs">
+  <title>General Functions</title>
+
+  <variablelist>
+   <varlistentry id="pglogicalinspect-funcs-pg-get-logical-snapshot-meta">
+    <term>
+     <function>pg_get_logical_snapshot_meta(filename text) returns record</function>
+    </term>
+
+    <listitem>
+     <para>
+      Gets logical snapshot metadata about a snapshot file that is located in
+      the server's <filename>pg_logical/snapshots</filename> directory.
+      The <replaceable>filename</replaceable> argument represents the snapshot
+      file name.
+      For example:
+<screen>
+postgres=# SELECT * FROM pg_ls_logicalsnapdir();
+-[ RECORD 1 ]+-----------------------
+name         | 0-40796E18.snap
+size         | 152
+modification | 2024-08-14 16:36:32+00
+
+postgres=# SELECT * FROM pg_get_logical_snapshot_meta('0-40796E18.snap');
+-[ RECORD 1 ]--------
+magic    | 1369563137
+checksum | 1028045905
+version  | 6
+
+postgres=# SELECT ss.name, meta.* FROM pg_ls_logicalsnapdir() AS ss,
+pg_get_logical_snapshot_meta(ss.name) AS meta;
+-[ RECORD 1 ]-------------
+name     | 0-40796E18.snap
+magic    | 1369563137
+checksum | 1028045905
+version  | 6
+</screen>
+     </para>
+     <para>
+      If <replaceable>filename</replaceable> does not match a snapshot file, the
+      function raises an error.
+     </para>
+    </listitem>
+   </varlistentry>
+
+   <varlistentry id="pglogicalinspect-funcs-pg-get-logical-snapshot-info">
+    <term>
+     <function>pg_get_logical_snapshot_info(filename text) returns record</function>
+    </term>
+
+    <listitem>
+     <para>
+      Gets logical snapshot information about a snapshot file that is located in
+      the server's <filename>pg_logical/snapshots</filename> directory.
+      The <replaceable>filename</replaceable> argument represents the snapshot
+      file name.
+      For example:
+<screen>
+postgres=# SELECT * FROM pg_ls_logicalsnapdir();
+-[ RECORD 1 ]+-----------------------
+name         | 0-40796E18.snap
+size         | 152
+modification | 2024-08-14 16:36:32+00
+
+postgres=# SELECT * FROM pg_get_logical_snapshot_info('0-40796E18.snap');
+-[ RECORD 1 ]------------+-----------
+state                    | consistent
+xmin                     | 751
+xmax                     | 751
+start_decoding_at        | 0/40796AF8
+two_phase_at             | 0/40796AF8
+initial_xmin_horizon     | 0
+building_full_snapshot   | f
+in_slot_creation         | f
+last_serialized_snapshot | 0/0
+next_phase_at            | 0
+committed_count          | 0
+committed_xip            |
+catchange_count          | 2
+catchange_xip            | {751,752}
+
+postgres=# SELECT ss.name, info.* FROM pg_ls_logicalsnapdir() AS ss,
+pg_get_logical_snapshot_info(ss.name) AS info;
+-[ RECORD 1 ]------------+----------------
+name                     | 0-40796E18.snap
+state                    | consistent
+xmin                     | 751
+xmax                     | 751
+start_decoding_at        | 0/40796AF8
+two_phase_at             | 0/40796AF8
+initial_xmin_horizon     | 0
+building_full_snapshot   | f
+in_slot_creation         | f
+last_serialized_snapshot | 0/0
+next_phase_at            | 0
+committed_count          | 0
+committed_xip            |
+catchange_count          | 2
+catchange_xip            | {751,752}
+</screen>
+     </para>
+     <para>
+      If <replaceable>filename</replaceable> does not match a snapshot file, the
+      function raises an error.
+     </para>
+    </listitem>
+   </varlistentry>
+
+  </variablelist>
+ </sect2>
+
+ <sect2 id="pglogicalinspect-author">
+  <title>Author</title>
+
+  <para>
+   Bertrand Drouvot <email>[email-masked]</email>
+  </para>
+ </sect2>
+
+</sect1>
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 0450f94ba8..c197198b87 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -134,6 +134,7 @@
 #include "replication/logical.h"
 #include "replication/reorderbuffer.h"
 #include "replication/snapbuild.h"
+#include "replication/snapbuild_internal.h"
 #include "storage/fd.h"
 #include "storage/lmgr.h"
 #include "storage/proc.h"
@@ -143,146 +144,6 @@
 #include "utils/memutils.h"
 #include "utils/snapmgr.h"
 #include "utils/snapshot.h"
-
-/*
- * This struct contains the current state of the snapshot building
- * machinery. Besides a forward declaration in the header, it is not exposed
- * to the public, so we can easily change its contents.
- */
-struct SnapBuild
-{
-	/* how far are we along building our first full snapshot */
-	SnapBuildState state;
-
-	/* private memory context used to allocate memory for this module. */
-	MemoryContext context;
-
-	/* all transactions < than this have committed/aborted */
-	TransactionId xmin;
-
-	/* all transactions >= than this are uncommitted */
-	TransactionId xmax;
-
-	/*
-	 * Don't replay commits from an LSN < this LSN. This can be set externally
-	 * but it will also be advanced (never retreat) from within snapbuild.c.
-	 */
-	XLogRecPtr	start_decoding_at;
-
-	/*
-	 * LSN at which two-phase decoding was enabled or LSN at which we found a
-	 * consistent point at the time of slot creation.
-	 *
-	 * The prepared transactions, that were skipped because previously
-	 * two-phase was not enabled or are not covered by initial snapshot, need
-	 * to be sent later along with commit prepared and they must be before
-	 * this point.
-	 */
-	XLogRecPtr	two_phase_at;
-
-	/*
-	 * Don't start decoding WAL until the "xl_running_xacts" information
-	 * indicates there are no running xids with an xid smaller than this.
-	 */
-	TransactionId initial_xmin_horizon;
-
-	/* Indicates if we are building full snapshot or just catalog one. */
-	bool		building_full_snapshot;
-
-	/*
-	 * Indicates if we are using the snapshot builder for the creation of a
-	 * logical replication slot. If it's true, the start point for decoding
-	 * changes is not determined yet. So we skip snapshot restores to properly
-	 * find the start point. See SnapBuildFindSnapshot() for details.
-	 */
-	bool		in_slot_creation;
-
-	/*
-	 * Snapshot that's valid to see the catalog state seen at this moment.
-	 */
-	Snapshot	snapshot;
-
-	/*
-	 * LSN of the last location we are sure a snapshot has been serialized to.
-	 */
-	XLogRecPtr	last_serialized_snapshot;
-
-	/*
-	 * The reorderbuffer we need to update with usable snapshots et al.
-	 */
-	ReorderBuffer *reorder;
-
-	/*
-	 * TransactionId at which the next phase of initial snapshot building will
-	 * happen. InvalidTransactionId if not known (i.e. SNAPBUILD_START), or
-	 * when no next phase necessary (SNAPBUILD_CONSISTENT).
-	 */
-	TransactionId next_phase_at;
-
-	/*
-	 * Array of transactions which could have catalog changes that committed
-	 * between xmin and xmax.
-	 */
-	struct
-	{
-		/* number of committed transactions */
-		size_t		xcnt;
-
-		/* available space for committed transactions */
-		size_t		xcnt_space;
-
-		/*
-		 * Until we reach a CONSISTENT state, we record commits of all
-		 * transactions, not just the catalog changing ones. Record when that
-		 * changes so we know we cannot export a snapshot safely anymore.
-		 */
-		bool		includes_all_transactions;
-
-		/*
-		 * Array of committed transactions that have modified the catalog.
-		 *
-		 * As this array is frequently modified we do *not* keep it in
-		 * xidComparator order. Instead we sort the array when building &
-		 * distributing a snapshot.
-		 *
-		 * TODO: It's unclear whether that reasoning has much merit. Every
-		 * time we add something here after becoming consistent will also
-		 * require distributing a snapshot. Storing them sorted would
-		 * potentially also make it easier to purge (but more complicated wrt
-		 * wraparound?). Should be improved if sorting while building the
-		 * snapshot shows up in profiles.
-		 */
-		TransactionId *xip;
-	}			committed;
-
-	/*
-	 * Array of transactions and subtransactions that had modified catalogs
-	 * and were running when the snapshot was serialized.
-	 *
-	 * We normally rely on some WAL record types such as HEAP2_NEW_CID to know
-	 * if the transaction has changed the catalog. But it could happen that
-	 * the logical decoding decodes only the commit record of the transaction
-	 * after restoring the previously serialized snapshot in which case we
-	 * will miss adding the xid to the snapshot and end up looking at the
-	 * catalogs with the wrong snapshot.
-	 *
-	 * Now to avoid the above problem, we serialize the transactions that had
-	 * modified the catalogs and are still running at the time of snapshot
-	 * serialization. We fill this array while restoring the snapshot and then
-	 * refer it while decoding commit to ensure if the xact has modified the
-	 * catalog. We discard this array when all the xids in the list become old
-	 * enough to matter. See SnapBuildPurgeOlderTxn for details.
-	 */
-	struct
-	{
-		/* number of transactions */
-		size_t		xcnt;
-
-		/* This array must be sorted in xidComparator order */
-		TransactionId *xip;
-	}			catchange;
-};
-
 /*
  * Starting a transaction -- which we need to do while exporting a snapshot --
  * removes knowledge about the previously used resowner, so we save it here.
@@ -1557,40 +1418,6 @@ SnapBuildWaitSnapshot(xl_running_xacts *running, TransactionId cutoff)
 	}
 }
 
-/* -----------------------------------
- * Snapshot serialization support
- * -----------------------------------
- */
-
-/*
- * We store current state of struct SnapBuild on disk in the following manner:
- *
- * struct SnapBuildOnDisk;
- * TransactionId * committed.xcnt; (*not xcnt_space*)
- * TransactionId * catchange.xcnt;
- *
- */
-typedef struct SnapBuildOnDisk
-{
-	/* first part of this struct needs to be version independent */
-
-	/* data not covered by checksum */
-	uint32		magic;
-	pg_crc32c	checksum;
-
-	/* data covered by checksum */
-
-	/* version, in case we want to support pg_upgrade */
-	uint32		version;
-	/* how large is the on disk data, excluding the constant sized part */
-	uint32		length;
-
-	/* version dependent part */
-	SnapBuild	builder;
-
-	/* variable amount of TransactionIds follows */
-} SnapBuildOnDisk;
-
 #define SnapBuildOnDiskConstantSize \
 	offsetof(SnapBuildOnDisk, builder)
 #define SnapBuildOnDiskNotChecksummedSize \
@@ -1857,34 +1684,27 @@ out:
 }
 
 /*
- * Restore a snapshot into 'builder' if previously one has been stored at the
- * location indicated by 'lsn'. Returns true if successful, false otherwise.
+ * Validate the logical snapshot file and read its contents to 'ondisk'.
  */
-static bool
-SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
+bool
+ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk, const char *path,
+							   MemoryContext context, bool missing_ok)
 {
-	SnapBuildOnDisk ondisk;
 	int			fd;
-	char		path[MAXPGPATH];
-	Size		sz;
 	pg_crc32c	checksum;
-
-	/* no point in loading a snapshot if we're already there */
-	if (builder->state == SNAPBUILD_CONSISTENT)
-		return false;
-
-	sprintf(path, "%s/%X-%X.snap",
-			PG_LOGICAL_SNAPSHOTS_DIR,
-			LSN_FORMAT_ARGS(lsn));
+	Size		sz;

-	if (fd < 0 && errno == ENOENT)
-		return false;
-	else if (fd < 0)
+	if (fd < 0)
+	{
+		if (missing_ok && errno == ENOENT)
+			return false;
+
 		ereport(ERROR,
 				(errcode_for_file_access(),
 				 errmsg("could not open file \"%s\": %m", path)));
+	}
 
 	/* ----
 	 * Make sure the snapshot had been stored safely to disk, that's normally
@@ -1897,47 +1717,46 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	fsync_fname(path, false);
 	fsync_fname(PG_LOGICAL_SNAPSHOTS_DIR, true);
 
-
 	/* read statically sized portion of snapshot */
-	SnapBuildRestoreContents(fd, (char *) &ondisk, SnapBuildOnDiskConstantSize, path);
+	SnapBuildRestoreContents(fd, (char *) ondisk, SnapBuildOnDiskConstantSize, path);
 
-	if (ondisk.magic != SNAPBUILD_MAGIC)
+	if (ondisk->magic != SNAPBUILD_MAGIC)
 		ereport(ERROR,
 				(errcode(ERRCODE_DATA_CORRUPTED),
 				 errmsg("snapbuild state file \"%s\" has wrong magic number: %u instead of %u",
-						path, ondisk.magic, SNAPBUILD_MAGIC)));
+						path, ondisk->magic, SNAPBUILD_MAGIC)));
 
-	if (ondisk.version != SNAPBUILD_VERSION)
+	if (ondisk->version != SNAPBUILD_VERSION)
 		ereport(ERROR,
 				(errcode(ERRCODE_DATA_CORRUPTED),
 				 errmsg("snapbuild state file \"%s\" has unsupported version: %u instead of %u",
-						path, ondisk.version, SNAPBUILD_VERSION)));
+						path, ondisk->version, SNAPBUILD_VERSION)));
 
 	INIT_CRC32C(checksum);
 	COMP_CRC32C(checksum,
-				((char *) &ondisk) + SnapBuildOnDiskNotChecksummedSize,
+				((char *) ondisk) + SnapBuildOnDiskNotChecksummedSize,
 				SnapBuildOnDiskConstantSize - SnapBuildOnDiskNotChecksummedSize);
 
 	/* read SnapBuild */
-	SnapBuildRestoreContents(fd, (char *) &ondisk.builder, sizeof(SnapBuild), path);
-	COMP_CRC32C(checksum, &ondisk.builder, sizeof(SnapBuild));
+	SnapBuildRestoreContents(fd, (char *) &ondisk->builder, sizeof(SnapBuild), path);
+	COMP_CRC32C(checksum, &ondisk->builder, sizeof(SnapBuild));
 
 	/* restore committed xacts information */
-	if (ondisk.builder.committed.xcnt > 0)
+	if (ondisk->builder.committed.xcnt > 0)
 	{
-		sz = sizeof(TransactionId) * ondisk.builder.committed.xcnt;
-		ondisk.builder.committed.xip = MemoryContextAllocZero(builder->context, sz);
-		SnapBuildRestoreContents(fd, (char *) ondisk.builder.committed.xip, sz, path);
-		COMP_CRC32C(checksum, ondisk.builder.committed.xip, sz);
+		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);
 	}
 
 	/* restore catalog modifying xacts information */
-	if (ondisk.builder.catchange.xcnt > 0)
+	if (ondisk->builder.catchange.xcnt > 0)
 	{
-		sz = sizeof(TransactionId) * ondisk.builder.catchange.xcnt;
-		ondisk.builder.catchange.xip = MemoryContextAllocZero(builder->context, sz);
-		SnapBuildRestoreContents(fd, (char *) ondisk.builder.catchange.xip, sz, path);
-		COMP_CRC32C(checksum, ondisk.builder.catchange.xip, sz);
+		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);
 	}
 
 	if (CloseTransientFile(fd) != 0)
@@ -1948,11 +1767,36 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	FIN_CRC32C(checksum);
 
 	/* verify checksum of what we've read */
-	if (!EQ_CRC32C(checksum, ondisk.checksum))
+	if (!EQ_CRC32C(checksum, ondisk->checksum))
 		ereport(ERROR,
 				(errcode(ERRCODE_DATA_CORRUPTED),
 				 errmsg("checksum mismatch for snapbuild state file \"%s\": is %u, should be %u",
-						path, checksum, ondisk.checksum)));
+						path, checksum, ondisk->checksum)));
+
+	return true;
+}
+
+/*
+ * Restore a snapshot into 'builder' if previously one has been stored at the
+ * location indicated by 'lsn'. Returns true if successful, false otherwise.
+ */
+static bool
+SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
+{
+	SnapBuildOnDisk ondisk;
+	char		path[MAXPGPATH];
+
+	/* no point in loading a snapshot if we're already there */
+	if (builder->state == SNAPBUILD_CONSISTENT)
+		return false;
+
+	sprintf(path, "%s/%X-%X.snap",
+			PG_LOGICAL_SNAPSHOTS_DIR,
+			LSN_FORMAT_ARGS(lsn));
+
+	/* validate and restore the snapshot to 'ondisk' */
+	if (!ValidateAndRestoreSnapshotFile(&ondisk, path, builder->context, true))
+		return false;
 
 	/*
 	 * ok, we now have a sensible snapshot here, figure out if it has more
diff --git a/src/include/replication/snapbuild.h b/src/include/replication/snapbuild.h
index caa5113ff8..3c1454df99 100644
--- a/src/include/replication/snapbuild.h
+++ b/src/include/replication/snapbuild.h
@@ -15,6 +15,10 @@
 #include "access/xlogdefs.h"
 #include "utils/snapmgr.h"
 
+/*
+ * Please keep get_snapbuild_state_desc() (located in the pg_logicalinspect
+ * module) updated if a change needs to be made to SnapBuildState.
+ */
 typedef enum
 {
 	/*
@@ -46,7 +50,7 @@ typedef enum
 	SNAPBUILD_CONSISTENT = 2,
 } SnapBuildState;
 
-/* forward declare so we don't have to expose the struct to the public */
+/* forward declare so we don't have to include snapbuild_internal.h */
 struct SnapBuild;
 typedef struct SnapBuild SnapBuild;
 
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
new file mode 100644
index 0000000000..ade75c94f1
--- /dev/null
+++ b/src/include/replication/snapbuild_internal.h
@@ -0,0 +1,199 @@
+/*-------------------------------------------------------------------------
+ *
+ * snapbuild_internal.h
+ *    This file contains declarations for logical decoding utility
+ *    functions for internal use.
+ *
+ * Copyright (c) 2024, PostgreSQL Global Development Group
+ *
+ * src/include/replication/snapbuild_internal.h
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef SNAPBUILD_INTERNAL_H
+#define SNAPBUILD_INTERNAL_H
+
+#include "port/pg_crc32c.h"
+#include "replication/reorderbuffer.h"
+#include "replication/snapbuild.h"
+
+/*
+ * This struct contains the current state of the snapshot building
+ * machinery. It is exposed to the public, so pay attention when changing its
+ * contents.
+ */
+typedef struct SnapBuild
+{
+	/* how far are we along building our first full snapshot */
+	SnapBuildState state;
+
+	/* private memory context used to allocate memory for this module. */
+	MemoryContext context;
+
+	/* all transactions < than this have committed/aborted */
+	TransactionId xmin;
+
+	/* all transactions >= than this are uncommitted */
+	TransactionId xmax;
+
+	/*
+	 * Don't replay commits from an LSN < this LSN. This can be set externally
+	 * but it will also be advanced (never retreat) from within snapbuild.c.
+	 */
+	XLogRecPtr	start_decoding_at;
+
+	/*
+	 * LSN at which two-phase decoding was enabled or LSN at which we found a
+	 * consistent point at the time of slot creation.
+	 *
+	 * The prepared transactions, that were skipped because previously
+	 * two-phase was not enabled or are not covered by initial snapshot, need
+	 * to be sent later along with commit prepared and they must be before
+	 * this point.
+	 */
+	XLogRecPtr	two_phase_at;
+
+	/*
+	 * Don't start decoding WAL until the "xl_running_xacts" information
+	 * indicates there are no running xids with an xid smaller than this.
+	 */
+	TransactionId initial_xmin_horizon;
+
+	/* Indicates if we are building full snapshot or just catalog one. */
+	bool		building_full_snapshot;
+
+	/*
+	 * Indicates if we are using the snapshot builder for the creation of a
+	 * logical replication slot. If it's true, the start point for decoding
+	 * changes is not determined yet. So we skip snapshot restores to properly
+	 * find the start point. See SnapBuildFindSnapshot() for details.
+	 */
+	bool		in_slot_creation;
+
+	/*
+	 * Snapshot that's valid to see the catalog state seen at this moment.
+	 */
+	Snapshot	snapshot;
+
+	/*
+	 * LSN of the last location we are sure a snapshot has been serialized to.
+	 */
+	XLogRecPtr	last_serialized_snapshot;
+
+	/*
+	 * The reorderbuffer we need to update with usable snapshots et al.
+	 */
+	ReorderBuffer *reorder;
+
+	/*
+	 * TransactionId at which the next phase of initial snapshot building will
+	 * happen. InvalidTransactionId if not known (i.e. SNAPBUILD_START), or
+	 * when no next phase necessary (SNAPBUILD_CONSISTENT).
+	 */
+	TransactionId next_phase_at;
+
+	/*
+	 * Array of transactions which could have catalog changes that committed
+	 * between xmin and xmax.
+	 */
+	struct
+	{
+		/* number of committed transactions */
+		size_t		xcnt;
+
+		/* available space for committed transactions */
+		size_t		xcnt_space;
+
+		/*
+		 * Until we reach a CONSISTENT state, we record commits of all
+		 * transactions, not just the catalog changing ones. Record when that
+		 * changes so we know we cannot export a snapshot safely anymore.
+		 */
+		bool		includes_all_transactions;
+
+		/*
+		 * Array of committed transactions that have modified the catalog.
+		 *
+		 * As this array is frequently modified we do *not* keep it in
+		 * xidComparator order. Instead we sort the array when building &
+		 * distributing a snapshot.
+		 *
+		 * TODO: It's unclear whether that reasoning has much merit. Every
+		 * time we add something here after becoming consistent will also
+		 * require distributing a snapshot. Storing them sorted would
+		 * potentially also make it easier to purge (but more complicated wrt
+		 * wraparound?). Should be improved if sorting while building the
+		 * snapshot shows up in profiles.
+		 */
+		TransactionId *xip;
+	}			committed;
+
+	/*
+	 * Array of transactions and subtransactions that had modified catalogs
+	 * and were running when the snapshot was serialized.
+	 *
+	 * We normally rely on some WAL record types such as HEAP2_NEW_CID to know
+	 * if the transaction has changed the catalog. But it could happen that
+	 * the logical decoding decodes only the commit record of the transaction
+	 * after restoring the previously serialized snapshot in which case we
+	 * will miss adding the xid to the snapshot and end up looking at the
+	 * catalogs with the wrong snapshot.
+	 *
+	 * Now to avoid the above problem, we serialize the transactions that had
+	 * modified the catalogs and are still running at the time of snapshot
+	 * serialization. We fill this array while restoring the snapshot and then
+	 * refer it while decoding commit to ensure if the xact has modified the
+	 * catalog. We discard this array when all the xids in the list become old
+	 * enough to matter. See SnapBuildPurgeOlderTxn for details.
+	 */
+	struct
+	{
+		/* number of transactions */
+		size_t		xcnt;
+
+		/* This array must be sorted in xidComparator order */
+		TransactionId *xip;
+	}			catchange;
+} SnapBuild;
+
+/* -----------------------------------
+ * Snapshot serialization support
+ * -----------------------------------
+ */
+
+/*
+ * We store current state of struct SnapBuild on disk in the following manner:
+ *
+ * struct SnapBuildOnDisk;
+ * TransactionId * committed.xcnt; (*not xcnt_space*)
+ * TransactionId * catchange.xcnt;
+ *
+ * Check if the SnapBuildOnDiskConstantSize and SnapBuildOnDiskNotChecksummedSize
+ * macros need to be updated when modifying the SnapBuildOnDisk struct.
+ */
+typedef struct SnapBuildOnDisk
+{
+	/* first part of this struct needs to be version independent */
+
+	/* data not covered by checksum */
+	uint32		magic;
+	pg_crc32c	checksum;
+
+	/* data covered by checksum */
+
+	/* version, in case we want to support pg_upgrade */
+	uint32		version;
+	/* how large is the on disk data, excluding the constant sized part */
+	uint32		length;
+
+	/* version dependent part */
+	SnapBuild	builder;
+
+	/* variable amount of TransactionIds follows */
+} SnapBuildOnDisk;
+
+extern bool ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk, const char *path,
+										   MemoryContext context, bool missing_ok);
+
+#endif							/* SNAPBUILD_INTERNAL_H */
-- 
2.34.1

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 10 Oct 2024 17:38:43 -0700

> XIDOID and construct_array_builtin() and 0002 for the module itself).

Thank you for updating the patch!

> postgres=# select typbyval, typalign from pg_type where typname = 'xid';

+1

The patches mostly look good to me. Here are some minor comments:

+       sprintf(path, "%s/%s",
+                       PG_LOGICAL_SNAPSHOTS_DIR,
+                       text_to_cstring(filename_t));
+
+       /* Validate and restore the snapshot to 'ondisk' */
+       ValidateAndRestoreSnapshotFile(&ondisk, path,
CurrentMemoryContext, false);
+
+       /* Build a tuple descriptor for our result type */
+       if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+               elog(ERROR, "return type must be a row type");
+
I think it would be better to check the result type before reading the
snapshot file.

---

Why is only checksum casted to int64? With that, it can show a
checksum value as a non-netagive integer but is it really necessary?
For instance, page_header() function in pageinspect shows a page
checksum as smallint.

---@@ -0,0 +1,4 @@

output_iso and tmp_check_iso should be added.

---
 /*

I think it would be better to add some descriptions of the function
arguments, particularly context and missing_ok.

The names of all other functions in snapbuild.c have the "SnapBuild"
prefix. I think it's better to follow it. How about renaming it to
SnapBuildReadSnapshot(), SnapBuildRestoreSnapshot(), or something
along those lines?

Regards,

---

From: Peter Smith <[email-masked]>
Date: Fri, 11 Oct 2024 12:59:33 +1100

Hi, Here are a few comments for patch set v13*

//////////

Patch v13-0001

======
Commit message

1.1
/were no use case/was no use case/

~~~

1.2
It seemed a bit odd that the switch cases for
'construct_array_builtin' are not the same as those for
'deconstruct_array_builtin'.

For example, all these ones seem missing from deconstruct:
    case INT4OID:
    case INT8OID:
    case NAMEOID:
    case REGTYPEOID:

I know that has nothing to do with your patch, and I guess it does not
cause any problems otherwise there would be ERRORs. But, if you are to
follow this same current pattern, then perhaps you don't need to add
your new case for 'deconstruct_array_builtin', since AFAICT you are
never using it.

//////////

Patch v13-0002

======
pg_get_logical_snapshot_meta:

2.1

FWIW, if you wanted to avoid a few lines you could initialise the
nulls array during the declaration.
bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};

This seems a common pattern in other source code, and it replaces the
need for the subsequent memset.

~~~

pg_get_logical_snapshot_info:

2.2

Ditto of #2.1. You could instead just initialise in the declaration like:
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 11 Oct 2024 13:15:33 +0000

Hi,

> I think it would be better to check the result type before reading the

Agree, done in v14.

> For instance, page_header() function in pageinspect shows a page

Yeah, pd_checksum in PageHeaderData is uint16 while checksum in SnapBuildOnDisk
is pg_crc32c. The reason why it is casted to int64 is explained in [1], does that
make sense to you?

Yeah, done in v14.

> arguments, particularly context and missing_ok.

Thought about it but somehow managed to missed it. Thanks, done in v14.

> SnapBuildReadSnapshot(), SnapBuildRestoreSnapshot(), or something

That makes sense. I opted for SnapBuildRestoreSnapshot() (as it's calling
SnapBuildRestoreContents()). 

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

Regards,

>From 306d1d446773cf5d151fd2faa3b30df451c3dc1d Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Thu, 10 Oct 2024 04:09:53 +0000
Subject: [PATCH v14 1/2] Add XIDOID in construct_array_builtin()

d746021de1 (which introduced construct_array_builtin()) did not take care of
XIDOID because there was no use case at that time. Now that there is one, let's
add XIDOID.
---
 src/backend/utils/adt/arrayfuncs.c | 6 ++++++
 1 file changed, 6 insertions(+)
 100.0% src/backend/utils/adt/

index e5c7e57a5d..41434279c5 100644
 
+		case XIDOID:

>From 420616a91db9967901aaa09825b970f0775a3405 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Wed, 14 Aug 2024 08:46:05 +0000
Subject: [PATCH v14 2/2] Add contrib/pg_logicalinspect

It currently allows to inspect the contents of serialized logical snapshots of
a running database cluster, which is useful for debugging or educational
purposes.
---
 contrib/Makefile                              |   1 +
 contrib/meson.build                           |   1 +
 contrib/pg_logicalinspect/.gitignore          |   6 +
 contrib/pg_logicalinspect/Makefile            |  31 ++
 .../expected/logical_inspect.out              |  52 ++++
 contrib/pg_logicalinspect/logicalinspect.conf |   1 +
 contrib/pg_logicalinspect/meson.build         |  39 +++
 .../pg_logicalinspect--1.0.sql                |  43 +++
 contrib/pg_logicalinspect/pg_logicalinspect.c | 167 +++++++++++
 .../pg_logicalinspect.control                 |   5 +
 .../specs/logical_inspect.spec                |  34 +++
 doc/src/sgml/contrib.sgml                     |   1 +
 doc/src/sgml/filelist.sgml                    |   1 +
 doc/src/sgml/pglogicalinspect.sgml            | 143 +++++++++
 src/backend/replication/logical/snapbuild.c   | 274 ++++--------------
 src/include/replication/snapbuild.h           |   6 +-
 src/include/replication/snapbuild_internal.h  | 199 +++++++++++++
 17 files changed, 790 insertions(+), 214 deletions(-)
   7.6% contrib/pg_logicalinspect/expected/
   5.3% contrib/pg_logicalinspect/specs/
  26.0% contrib/pg_logicalinspect/
  14.0% doc/src/sgml/
  26.2% src/backend/replication/logical/
  20.5% src/include/replication/

index 0000000000..b4903eba65
@@ -0,0 +1,6 @@
+/output_iso/
+/tmp_check_iso/
index 0000000000..fabfe2a10c
@@ -0,0 +1,167 @@
+	Datum		values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
+	bool		nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
+	SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+	Datum		values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
+	bool		nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
+	SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
index 0450f94ba8..92fd57b77e 100644
 
@@ -1857,34 +1684,31 @@ out:
 }
 
 /*
- * Restore a snapshot into 'builder' if previously one has been stored at the
- * location indicated by 'lsn'. Returns true if successful, false otherwise.
+ * Restore the logical snapshot file contents to 'ondisk'.
+ *
+ * If 'missing_ok' is true, will not throw an error if the file is not found.
+ * 'context' is the memory context where the catalog modifying/committed xid
+ * will live.
  */
-static bool
-SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
+bool
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+						 MemoryContext context, bool missing_ok)
 {
-	SnapBuildOnDisk ondisk;
 	int			fd;
-	char		path[MAXPGPATH];
-	Size		sz;
 	pg_crc32c	checksum;
-
-	/* no point in loading a snapshot if we're already there */
-	if (builder->state == SNAPBUILD_CONSISTENT)
-		return false;
-
-	sprintf(path, "%s/%X-%X.snap",
-			PG_LOGICAL_SNAPSHOTS_DIR,
-			LSN_FORMAT_ARGS(lsn));
+	Size		sz;

 
 	/* ----
 	 * Make sure the snapshot had been stored safely to disk, that's normally
@@ -1897,47 +1721,46 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	fsync_fname(path, false);
 	fsync_fname(PG_LOGICAL_SNAPSHOTS_DIR, true);
 
-

 
 	INIT_CRC32C(checksum);
 	COMP_CRC32C(checksum,
 
 	/* read SnapBuild */

 
 	if (CloseTransientFile(fd) != 0)
@@ -1948,11 +1771,36 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	FIN_CRC32C(checksum);
 
+	if (!SnapBuildRestoreSnapshot(&ondisk, path, builder->context, true))
+		return false;
 
 	/*
 
+/*

index 0000000000..7134b48b96
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 11 Oct 2024 13:17:58 +0000

Hi,

> Hi, Here are a few comments for patch set v13*

Thanks for looking at it.

Updated in v14 just shared up-thread.

> your new case for 'deconstruct_array_builtin', since AFAICT you are

That's right. Strict pairing between deconstruct_array_builtin() and 
construct_array_builtin() is not required, let's remove this extra switch in
deconstruct_array_builtin() for code consistency (done in v14).

Okay, fine by me, let's do it for the "values" too in passing (this seems also
a common pattern), in v14.

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 11 Oct 2024 11:15:46 -0700

In the email, you said:

> As the checksum could be > 2^31 - 1, then v9 (just shared up-thread) changes it
> to an int8 in the pg_logicalinspect--1.0.sql file. So, to avoid CI failure on
> the 32bit build, then v9 is using Int64GetDatum() instead of UInt32GetDatum().

I'm fine with using Int64GetDatum() for checksum.

>
> > Same goes for below:
> > values[i++] = Int32GetDatum(ondisk.magic);
> > values[i++] = Int32GetDatum(ondisk.magic);
>
> The 2 others field (magic and version) are unlikely to be > 2^31 - 1, so v9 is
> making use of UInt32GetDatum() and keep int4 in the sql file.

While I agree that these two fields are unlikely to be > 2^31 - 1, I'm
concerned a bit about an inconsistency that the patch uses
Int64GetDatum also for both ondisk.builder.committed.xcnt and
ondisk.builder.catchange.xcnt.

I have a minor comment:

If we use "General Functions" here it sounds like there are other
functions for specific purposes in pg_logicalinspect module. How about
using "Functions" instead?

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 11 Oct 2024 16:48:26 -0700

> using "Functions" instead?

To elaborate further, pageinspect has a "General Functions" section,
which makes sense to me as it has other AM-type specific functions. On
the other hand, pg_logicalinspect has SQL functions only for one
logical replication component. So I think it makes sense to use
"Function" instead. pg_walinspect also has the sole section "General
Function" but I personally think that "Function" is more appropriate
like other modules does.

BTW I think that adding snapshot_internal.h could be a separate patch.
That makes the main pg_logicalinspect patch cleaner.

I've attached updated patches with some minor changes, and done the patch split.

The 0001 patch just moves both SnapBuild and SnapBuildOnDisk structs
to snapshot_internal.h. The 0002 is the main pg_logicalinspect patch.
I've merged the previous Add-XIDOID-in-construct_array_builtin.patch
to the main patch. The minor_change.patch.txt is the difference I
added on top of v14 patch set.

I'm going to push them early next week, barring any objections and
further comments.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
commit 483f5a402b6f650d02e8f8668bd50b17680f2805
Author: Masahiko Sawada <[email-masked]>
Date:   Fri Oct 11 15:52:49 2024 -0700

    minor changes

diff --git a/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql b/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
index c773f6e458..8f7f947cbb 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
+++ b/contrib/pg_logicalinspect/pg_logicalinspect--1.0.sql
@@ -31,9 +31,9 @@ CREATE FUNCTION pg_get_logical_snapshot_info(IN filename text,
     OUT in_slot_creation boolean,
     OUT last_serialized_snapshot pg_lsn,
     OUT next_phase_at xid,
-    OUT committed_count int8,
+    OUT committed_count int4,
     OUT committed_xip xid[],
-    OUT catchange_count int8,
+    OUT catchange_count int4,
     OUT catchange_xip xid[]
 )
 AS 'MODULE_PATHNAME', 'pg_get_logical_snapshot_info'
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index fabfe2a10c..790c64d6fa 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -123,7 +123,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
 	values[i++] = LSNGetDatum(ondisk.builder.last_serialized_snapshot);
 	values[i++] = TransactionIdGetDatum(ondisk.builder.next_phase_at);
 
-	values[i++] = Int64GetDatum(ondisk.builder.committed.xcnt);
+	values[i++] = UInt32GetDatum(ondisk.builder.committed.xcnt);
 	if (ondisk.builder.committed.xcnt > 0)
 	{
 		Datum	   *arrayelems;
@@ -140,7 +140,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
 	else
 		nulls[i++] = true;
 
-	values[i++] = Int64GetDatum(ondisk.builder.catchange.xcnt);
+	values[i++] = UInt32GetDatum(ondisk.builder.catchange.xcnt);
 	if (ondisk.builder.catchange.xcnt > 0)
 	{
 		Datum	   *arrayelems;
diff --git a/doc/src/sgml/pglogicalinspect.sgml b/doc/src/sgml/pglogicalinspect.sgml
index e0fac997b6..4b111f9611 100644
--- a/doc/src/sgml/pglogicalinspect.sgml
+++ b/doc/src/sgml/pglogicalinspect.sgml
@@ -22,7 +22,7 @@
  </para>
 
  <sect2 id="pglogicalinspect-funcs">
-  <title>General Functions</title>
+  <title>Functions</title>
 
   <variablelist>
    <varlistentry id="pglogicalinspect-funcs-pg-get-logical-snapshot-meta">

---

From: Peter Smith <[email-masked]>
Date: Mon, 14 Oct 2024 09:57:22 +1100

Here are some minor review comments for v15-0002.

======
contrib/pg_logicalinspect/pg_logicalinspect.c

1.

The sprintf should be deferred. Could you do it after the ERROR check?

~~~

2.

Ditto #1. The sprintf should be deferred. Could you do it after the ERROR check?

======

3.
 /*

nit - I think it's better to describe parameters in the same order
that they are declared. Also, include a 'path' description, so it is
not the only one omitted.

SUGGESTION:
'path' - snapshot file path.
'context' - memory context where the catalog modifying/committed xid will live.
‘missing_ok’ – when true, don't throw an error if the file is not found.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 14 Oct 2024 06:17:38 +0000

Hi,

> > ondisk.builder.catchange.xcnt.

Thanks for the feedback. That makes sense and I agree with the proposal done
in v15.

> Function"

Yeah, I used it as a "template".

> but I personally think that "Function" is more appropriate
> like other modules does.

I do agree.

> That makes the main pg_logicalinspect patch cleaner.

Agree.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 14 Oct 2024 06:23:06 +0000

Hi,

> The sprintf should be deferred. Could you do it after the ERROR check?

I think that makes sense, done in v16 attached.

> that they are declared.

Done in v16.

> Also, include a 'path' description, so it is

I don't think that's worth it as self explanatory IMHO.

Regards,

>From ef5997544afd9f414648e5ccae6c0d08c5ab66fa Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <[email-masked]>
Date: Fri, 11 Oct 2024 16:23:14 -0700
Subject: [PATCH v16 1/2] Move SnapBuild and SnapBuildOnDisk structs to
 snapshot_internal.h

This commit moves the definitions of the SnapBuild and SnapBuildOnDisk
structs, related to logical snapshots, to the snapshot_internal.h
file. This change allows external tools, such as
pg_logicalinspect (with an upcoming patch), to access and utilize the
contents of logical snapshots.

Author: Bertrand Drouvot
Reviewed-by: Amit Kapila, Shveta Malik, Peter Smith
Discussion: https://postgr.es/m/ZscuZ92uGh3wm4tW%40ip-10-97-1-34.eu-west-3.compute.internal
---
 src/backend/replication/logical/snapbuild.c  | 175 +----------------
 src/include/replication/snapbuild.h          |   2 +-
 src/include/replication/snapbuild_internal.h | 196 +++++++++++++++++++
 3 files changed, 198 insertions(+), 175 deletions(-)
  46.4% src/backend/replication/logical/
  53.5% src/include/replication/

index 0450f94ba8..b9df8c0a02 100644
 
index caa5113ff8..dbb4bc2f4b 100644
@@ -46,7 +46,7 @@ typedef enum

index 0000000000..03719ccf2a
@@ -0,0 +1,196 @@

>From 9f9e2ed520d0e9315dc16521f2f7d1e26fc50cb7 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <[email-masked]>
Date: Fri, 11 Oct 2024 16:24:14 -0700
Subject: [PATCH v16 2/2] Add contrib/pg_logicalinspect.

This module provides SQL functions that allow to inspect logical
decoding components.

It currently allows to inspect the contents of serialized logical
snapshots of a running database cluster, which is useful for debugging
or educational purposes.

Author: Bertrand Drouvot
Reviewed-by: Amit Kapila, Shveta Malik, Peter Smith, Peter Eisentraut
Reviewed-by: David G. Johnston
Discussion: https://postgr.es/m/ZscuZ92uGh3wm4tW%40ip-10-97-1-34.eu-west-3.compute.internal
---
 contrib/Makefile                              |   1 +
 contrib/meson.build                           |   1 +
 contrib/pg_logicalinspect/.gitignore          |   6 +
 contrib/pg_logicalinspect/Makefile            |  31 ++++
 .../expected/logical_inspect.out              |  52 ++++++
 contrib/pg_logicalinspect/logicalinspect.conf |   1 +
 contrib/pg_logicalinspect/meson.build         |  39 ++++
 .../pg_logicalinspect--1.0.sql                |  43 +++++
 contrib/pg_logicalinspect/pg_logicalinspect.c | 167 ++++++++++++++++++
 .../pg_logicalinspect.control                 |   5 +
 .../specs/logical_inspect.spec                |  34 ++++
 doc/src/sgml/contrib.sgml                     |   1 +
 doc/src/sgml/filelist.sgml                    |   1 +
 doc/src/sgml/pglogicalinspect.sgml            | 143 +++++++++++++++
 src/backend/replication/logical/snapbuild.c   |  99 +++++++----
 src/backend/utils/adt/arrayfuncs.c            |   6 +
 src/include/replication/snapbuild.h           |   4 +
 src/include/replication/snapbuild_internal.h  |   3 +
 18 files changed, 598 insertions(+), 39 deletions(-)
  12.0% contrib/pg_logicalinspect/expected/
   8.4% contrib/pg_logicalinspect/specs/
  40.8% contrib/pg_logicalinspect/
  21.9% doc/src/sgml/
  14.5% src/backend/replication/logical/

index 0000000000..8f7f947cbb
index 0000000000..675760e686
index 0000000000..4b111f9611
index b9df8c0a02..a6a4da3266 100644
@@ -1684,34 +1684,31 @@ out:
 }
 
 /*

 
 	/* ----
 	 * Make sure the snapshot had been stored safely to disk, that's normally
@@ -1724,47 +1721,46 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	fsync_fname(path, false);
 	fsync_fname(PG_LOGICAL_SNAPSHOTS_DIR, true);
 
-

 
 	INIT_CRC32C(checksum);
 	COMP_CRC32C(checksum,
 
 	/* read SnapBuild */

 
 	if (CloseTransientFile(fd) != 0)
@@ -1775,11 +1771,36 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	FIN_CRC32C(checksum);

 	/*
 
+		case XIDOID:
index dbb4bc2f4b..3c1454df99 100644
 
+/*
+ * Please keep get_snapbuild_state_desc() (located in the pg_logicalinspect
+ * module) updated if a change needs to be made to SnapBuildState.
+ */
 typedef enum
 {
 	/*
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
index 03719ccf2a..7134b48b96 100644
--- a/src/include/replication/snapbuild_internal.h
+++ b/src/include/replication/snapbuild_internal.h
@@ -193,4 +193,7 @@ typedef struct SnapBuildOnDisk
 	/* variable amount of TransactionIds follows */
 } SnapBuildOnDisk;
 
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+									 MemoryContext context, bool missing_ok);
+
 #endif							/* SNAPBUILD_INTERNAL_H */
-- 
2.34.1

---

From: Peter Smith <[email-masked]>
Date: Mon, 14 Oct 2024 19:45:25 +1100

FYI - Although I did not re-apply/test the latest patchset v16*, by
visual inspection of the minor v15/v16 diffs it looks good to me.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 14 Oct 2024 18:08:10 -0700

> I don't think that's worth it as self explanatory IMHO.

Thank you for updating the patches!

I fixed a compiler warning by -Wtypedef-redefinition related to the
declaration of SnapBuild struct, then pushed both patches.

Regards,

---

From: Andres Freund <[email-masked]>
Date: Tue, 4 Mar 2025 16:56:22 -0500

Hi,

> declaration of SnapBuild struct, then pushed both patches.

This just failed on skink (valgrind buildfarm animal):
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-03-04%2017%3A35%3A01

In the last months (not sure quite how long) only the main regression tests
were running under valgrind. I fixed that, and in one of the runs since then
the above regression failure was triggered.

diff -U3 /home/bf/bf-build/skink-master/HEAD/pgsql/contrib/pg_logicalinspect/expected/logical_inspect.out /home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/pg_logicalinspect/isolation/results/logical_inspect.out
--- /home/bf/bf-build/skink-master/HEAD/pgsql/contrib/pg_logicalinspect/expected/logical_inspect.out	2024-10-15 01:07:04.632684683 +0000
+++ /home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/pg_logicalinspect/isolation/results/logical_inspect.out	2025-03-04 18:49:34.659306138 +0000
@@ -42,11 +42,12 @@
 ----------+---------------+----------------------+---------------+----------------------
 consistent|              0|                      |              2|                     2
 consistent|              2|                     2|              0|
-(2 rows)
+consistent|              2|                     2|              0|
+(3 rows)

 step s1_get_logical_snapshot_meta: SELECT COUNT(meta.*) from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;
 count
 -----
-    2
+    3
 (1 row)

Greetings,

Andres Freund

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 4 Mar 2025 22:25:57 -0800

> +++ /home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/pg_logicalinspect/isolation/results/logical_inspect.out     2025-03-04 18:49:34.659306138 +0000

Thank you for the report.

It seems that bgwriter wrote another RUNNING_XACTS record during the
test, making the logical decoding write an extra snapshot on the disk.

One way to stabilize the regression test would be that we check if
there is a serialized snapshot that has expected number of catchange
transactions and number of committed transactions, instead of dumping
all serialized snapshots.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 5 Mar 2025 07:17:09 +0000

Hi,

+1

> test, making the logical decoding write an extra snapshot on the disk.

Yup.

> all serialized snapshots.

Agree, PFA a patch doing so.

Regards,

>From d590d3184d4345908f1b033aa8a5d19cf98e88ba Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Wed, 5 Mar 2025 07:06:58 +0000
Subject: [PATCH v1] Modify pg_logicalinspect isolation test

The previous version was relying on the fact that the test produces exactly
2 snapshots on disk, while in fact it can produce more. Changing the test knowing
that at least 2 snapshots are generated.

Per buildfarm member skink.
---
 .../expected/logical_inspect.out              | 29 +++++++++++--------
 .../specs/logical_inspect.spec                |  7 +++--
 2 files changed, 21 insertions(+), 15 deletions(-)
  57.3% contrib/pg_logicalinspect/expected/
  42.6% contrib/pg_logicalinspect/specs/

diff --git a/contrib/pg_logicalinspect/expected/logical_inspect.out b/contrib/pg_logicalinspect/expected/logical_inspect.out
index d95efa4d1e5..ecba05b9ab1 100644
--- a/contrib/pg_logicalinspect/expected/logical_inspect.out
+++ b/contrib/pg_logicalinspect/expected/logical_inspect.out
@@ -1,6 +1,6 @@
 Parsed test spec with 2 sessions
 
-starting 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
+starting 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_catchange s1_get_logical_snapshot_info_committed s1_get_logical_snapshot_meta
 step s0_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding');
 ?column?
 --------
@@ -37,16 +37,21 @@ 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;
-state     |catchange_count|catchange_array_length|committed_count|committed_array_length
-----------+---------------+----------------------+---------------+----------------------
-consistent|              0|                      |              2|                     2
-consistent|              2|                     2|              0|                      
-(2 rows)
-
-step s1_get_logical_snapshot_meta: SELECT COUNT(meta.*) from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;
-count
------
-    2
+step s1_get_logical_snapshot_info_catchange: SELECT count(*) > 0 as has_catchange FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.catchange_count = 2 and array_length(info.catchange_xip,1) = 2 and info.committed_count = 0;
+has_catchange
+-------------
+t            
+(1 row)
+
+step s1_get_logical_snapshot_info_committed: SELECT count(*) > 0 as has_committed FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.committed_count = 2 and array_length(info.committed_xip,1) = 2 and info.catchange_count = 0;
+has_committed
+-------------
+t            
+(1 row)
+
+step s1_get_logical_snapshot_meta: SELECT COUNT(meta.*) > 1 AS has_meta from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;
+has_meta
+--------
+t       
 (1 row)
 
diff --git a/contrib/pg_logicalinspect/specs/logical_inspect.spec b/contrib/pg_logicalinspect/specs/logical_inspect.spec
index 9851a6c18e4..673d2f5ed0a 100644
--- a/contrib/pg_logicalinspect/specs/logical_inspect.spec
+++ b/contrib/pg_logicalinspect/specs/logical_inspect.spec
@@ -28,7 +28,8 @@ session "s1"
 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_meta" { SELECT COUNT(meta.*) > 1 AS has_meta from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta; }
+step "s1_get_logical_snapshot_info_catchange" { SELECT count(*) > 0 as has_catchange FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.catchange_count = 2 and array_length(info.catchange_xip,1) = 2 and info.committed_count = 0; }
+step "s1_get_logical_snapshot_info_committed" { SELECT count(*) > 0 as has_committed FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.committed_count = 2 and array_length(info.committed_xip,1) = 2 and info.catchange_count = 0; }
 
-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"
+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_catchange" "s1_get_logical_snapshot_info_committed" "s1_get_logical_snapshot_meta"
-- 
2.34.1

---

From: Amit Kapila <[email-masked]>
Date: Wed, 5 Mar 2025 14:42:15 +0530

> Agree, PFA a patch doing so.

It would be better if you could add a few comments atop the
permutation line to explain the working of the test. We have it for
other decoding-related tests. See
test_decoding/specs/subxact_without_top.spec for reference.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 5 Mar 2025 12:05:16 +0000

Hi,

On Wed, Mar 05, 2025 at 02:42:15PM +0530, Amit Kapila wrote:
> permutation line to explain the working of the test.

yeah makes sense. Done in the attached, and bonus point I realized that the
test could be simplified (so, removing useless steps in passing).

Regards,

>From 6154e847e4e5c2c7eb816d4302a4704d5a690954 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Wed, 5 Mar 2025 07:06:58 +0000
Subject: [PATCH v2] Modify pg_logicalinspect isolation test

In passing, removing useless steps and adding some comments.

Per buildfarm member skink.
---
 .../expected/logical_inspect.out              | 36 ++++++++-----------
 .../specs/logical_inspect.spec                | 11 +++---
 2 files changed, 22 insertions(+), 25 deletions(-)
  58.4% contrib/pg_logicalinspect/expected/
  41.5% contrib/pg_logicalinspect/specs/

index d95efa4d1e5..b7d01dbb68b 100644
 
-starting 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
+starting permutation: s0_init s0_begin s0_savepoint s0_truncate s1_checkpoint s1_get_changes s0_commit s1_checkpoint s1_get_changes s1_get_logical_snapshot_info_catchange s1_get_logical_snapshot_info_committed s1_get_logical_snapshot_meta
 step s0_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding');
 ?column?
 --------
@@ -17,8 +17,6 @@ data
 (0 rows)
 
 step s0_commit: COMMIT;
-step s0_begin: BEGIN;
-step s0_insert: INSERT INTO tbl1 VALUES (1);
 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');
 data                                   
@@ -28,25 +26,21 @@ table public.tbl1: TRUNCATE: (no-flags)
 COMMIT                                 
 (3 rows)
 
-step s0_commit: COMMIT;
-step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
-data                                                         
--------------------------------------------------------------
-BEGIN                                                        
-table public.tbl1: INSERT: val1[integer]:1 val2[integer]:null
-COMMIT                                                       
-(3 rows)
+step s1_get_logical_snapshot_info_catchange: SELECT count(*) > 0 as has_catchange FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.catchange_count = 2 and array_length(info.catchange_xip,1) = 2 and info.committed_count = 0;
+has_catchange
+-------------
+t            
+(1 row)

 
diff --git a/contrib/pg_logicalinspect/specs/logical_inspect.spec b/contrib/pg_logicalinspect/specs/logical_inspect.spec
index 9851a6c18e4..631daf5db6c 100644
--- a/contrib/pg_logicalinspect/specs/logical_inspect.spec
+++ b/contrib/pg_logicalinspect/specs/logical_inspect.spec
@@ -21,14 +21,17 @@ step "s0_init" { SELECT 'init' FROM pg_create_logical_replication_slot('isolatio
 step "s0_begin" { BEGIN; }
 step "s0_savepoint" { SAVEPOINT sp1; }
 step "s0_truncate" { TRUNCATE tbl1; }
-step "s0_insert" { INSERT INTO tbl1 VALUES (1); }
 step "s0_commit" { COMMIT; }
 
 session "s1"
 
-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"
+# The first get_changes produces (at least) one snapshot that contains 2 catchanges
+# (the truncate and its parent transaction). The second get_changes produces one
+# snapshot that contains the 2 transactions above as committed.
+permutation "s0_init" "s0_begin" "s0_savepoint" "s0_truncate" "s1_checkpoint" "s1_get_changes" "s0_commit" "s1_checkpoint" "s1_get_changes" "s1_get_logical_snapshot_info_catchange" "s1_get_logical_snapshot_info_committed" "s1_get_logical_snapshot_meta"
-- 
2.34.1

---

From: Tom Lane <[email-masked]>
Date: Wed, 05 Mar 2025 18:10:41 -0500

> test could be simplified (so, removing useless steps in passing).

Just a side note: tayra showed two instances of this failure today
[1][2].  That's not using valgrind.  I wonder if we changed something
else recently that would make this more probable?

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra&dt=2025-03-05%2021%3A36%3A40
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra&dt=2025-03-05%2013%3A42%3A17

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 5 Mar 2025 23:28:23 -0800

> else recently that would make this more probable?

I've observed the third failure. I read through recent commits but
have no idea what commit made this more probable. Comparing other
tests on the success case[1] and failure case[2], it seems that tayra
were slow overall. For instance, the 'build' and 'check' were 00:00:19
vs. 00:02:42 and 00:02:42 vs. 00:19:17, respectively. I'm not sure
what caused tayra to be slower overall recently.

Regards,

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra&dt=2025-03-05%2001%3A22%3A07
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tayra&dt=2025-03-05%2013%3A42%3A17

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 6 Mar 2025 08:34:23 +0000

Hi,

On Wed, Mar 05, 2025 at 11:28:23PM -0800, Masahiko Sawada wrote:
> > [1][2].  That's not using valgrind.

Thanks for the report!

> I wonder if we changed something
> I've observed the third failure.

I also did a "slow" test with the code tree at 7cdfeee320e and I can observe
the same "issue".

> I'm not sure

yeah, tayra being slower is what make the test failure more probable. I'm also
not sure as to why.

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 6 Mar 2025 13:48:46 -0800

> test could be simplified (so, removing useless steps in passing).

Thank you for the patch.

The new simplified test case can be pretty-formatted as:

init
begin
savepoint
truncate
                checkpoint-1
                get_changes-1
commit
                checkpoint-2
                get_changes-2
                info_catchange check
                info_committed check
                meta check

IIUC if another checkpoint happens between get_change-2 and the
subsequent checks, the first snapshot would be removed during the
checkpoint, resulting in a test failure. I think we could check the
snapshot files while one transaction keeps open. The more simplified
test case would be:

init
begin
savepoint
insert(cat-change)
                begin
                insert(cat-change)
                commit
                checkpoint
                get_changes
                info_catchange check
                info_committed check
                meta check
commit

In this test case, we would have at least one serialized snapshot that
has both cat-changes and committed txns. What do you think?

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Fri, 7 Mar 2025 10:26:23 +0530

> has both cat-changes and committed txns. What do you think?

Your proposed change in the test sounds better than what we have now
but I think we should also avoid autovacuum to perform analyze as that
may add additional counts. For test_decoding, we keep
autovacuum_naptime = 1d in logical.conf file, we can either use the
same here or simply keep autovacuum off.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 7 Mar 2025 10:42:17 +0000

Hi,

> > The new simplified test case can be pretty-formatted as:

Yes.

> > checkpoint, resulting in a test failure.

Good catch! Yeah you're right, thanks!

> I think we could check the

Indeed, I think that would prevent snapshots to be removed.

The attached ends up doing:

init
begin
savepoint
truncate table1
		       create table table2
               checkpoint
               get_changes
               info check
               meta check
commit

As the 2 ongoing catalog changes and the committed catalog change are part of the
same snapshot, then I grouped the catchanges and committed changes checks in the
same "info check".

> same here or simply keep autovacuum off.

When writing the attached, I initially added extra paranoia in the tests by
using ">=", does that also address your autovacuum concern?

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Fri, 7 Mar 2025 17:44:00 +0530

> using ">=", does that also address your autovacuum concern?

Yes, that will address the autovacuum concern.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 7 Mar 2025 12:09:35 -0800

> using ">=", does that also address your autovacuum concern?

Thank you for updating the patch. It looks mostly good to me. I've
made some cosmetic changes and attached the updated version.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Sat, 8 Mar 2025 07:58:19 +0000

Hi,

> made some cosmetic changes and attached the updated version.

LGTM, thanks!

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 11 Mar 2025 15:34:01 -0700

> > made some cosmetic changes and attached the updated version.

Pushed.

Regards,

---

From: "Euler Taveira" <[email-masked]>
Date: Thu, 13 Mar 2025 22:19:43 -0300

> Pushed.

pgindent is saying this commit included some extra tabs.

git diff
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index ff6c682679f..5a44718bea8 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -86,7 +86,7 @@ parse_error:
    ereport(ERROR,
            errmsg("invalid snapshot file name \"%s\"", filename));
 
-   return InvalidXLogRecPtr;                   /* keep compiler quiet */
+   return InvalidXLogRecPtr;   /* keep compiler quiet */
} 

--
Euler Taveira
EDB   https://www.enterprisedb.com/

---

From: Masahiko Sawada <[email-masked]>
Date: Sat, 15 Mar 2025 22:39:48 -0700

> +   return InvalidXLogRecPtr;   /* keep compiler quiet */

Yes, David fixed it in commit b955df44340.

Regards,