Re: Add contrib/pg_logicalsnapinspect

On Fri, Mar 7, 2025 at 3:19 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Mar 5, 2025 at 4:05 AM Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> 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
> > > <bertranddrouvot(dot)pg(at)gmail(dot)com> 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.