Subject: Re: Add contrib/pg_logicalsnapinspect

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

On Fri, Mar 7, 2025 at 3:19 AM Masahiko Sawada <[email-masked]> wrote:
>
> On Wed, Mar 5, 2025 at 4:05 AM Bertrand Drouvot
> <[email-masked]> wrote:
> >
> > Hi,
> >
> > On Wed, Mar 05, 2025 at 02:42:15PM +0530, Amit Kapila wrote:
> > > On Wed, Mar 5, 2025 at 12:47 PM Bertrand Drouvot
> > > <[email-masked]> wrote:
> > > >
> > > > 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.
> >
> > yeah makes sense. Done in the attached, and bonus point I realized that the
> > 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?
>

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,

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

---

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,

-- 
Masahiko Sawada

---

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,