Dear Amit,
>
> Right, I think this is a better idea. I have a few comments:
> 1.
> + /*
> + * In 035_standby_logical_decoding.pl, RUNNING_XACTS could move slots's
> + * xmin forward and cause random failures.
>
> No need to use test file name in code comments.
Fixed.
> 2. The comments atop wait_until_vacuum_can_remove can be changed to
> indicate that we will avoid logging running_xact with the help of
> injection points.
Comments were updated for the master. In back-branches, they were removed
because the risk was removed.
> 3.
> + # Note that from this point the checkpointer and bgwriter will wait before
> + # they write RUNNING_XACT record.
> + $node_primary->safe_psql('testdb',
> + "SELECT injection_points_attach('log-running-xacts', 'wait');");
>
> Isn't it better to use 'error' as the second parameter as we don't
> want to wait at this injection point?
Right, and the comment atop it was updated.
> 4.
> + # XXX If the instance does not attach 'log-running-xacts', the bgwriter
> + # pocess would generate RUNNING_XACTS record, so that the test would fail.
> + sleep(20);
>
> I think it is better to make a separate patch (as a first patch) for
> this so that it can be used as a reproducer. I suggest to use
> checkpoint as used by one of Bertrand's patches to ensure that the
> issue reproduces in every environment.
Reproducer was separated to the .txt file.
> > Sadly IS_INJECTION_POINT_ATTACHED() was introduced for PG18 so that the
> patch
> > could not backport for PG17 as-is.
> >
>
> We can use 'wait' mode API in PG17 as used in one of the tests
> (injection_points_attach('heap_update-before-pin', 'wait');) but I
> think it may be better to just leave testing active slots in
> backbranches because anyway the new development happens on HEAD and we
> want to ensure that no breakage happens there.
OK. I've attached a patch for PG17 as well. Commit messages for them were also
updated.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
