Thread: Re: Fix 035_standby_logical_decoding.pl race conditions

On Mon, Feb 10, 2025 at 8:12 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Please find attached a patch to $SUBJECT.
>
> In rare circumstances (and on slow machines) it is possible that a xl_running_xacts
> is emitted and that the catalog_xmin of a logical slot on the standby advances
> past the conflict point. In that case, no conflict is reported and the test
> fails. It has been observed several times and the last discussion can be found
> in [1].
>

Is my understanding correct that bgwriter on primary node has created
a  xl_running_xacts, then that record is replicated to standby, and
while decoding it (xl_running_xacts) on standby via active_slot, we
advanced the catalog_xmin of active_slot? If this happens then the
replay of vacuum record on standby won't be able to invalidate the
active slot, right?

So, if the above is correct, the reason for generating extra
xl_running_xacts on primary is Vacuum followed by Insert on primary
via below part of test:
$node_primary->safe_psql(
'testdb', qq[VACUUM $vac_option verbose $to_vac;
  INSERT INTO flush_wal DEFAULT VALUES;]);

> To avoid the race condition to occur this commit adds an injection point to prevent
> the catalog_xmin of a logical slot to advance past the conflict point.
>
> While working on this patch, some adjustements have been needed for injection
> points (they are proposed in 0001):
>
> - Adds the ability to wakeup() and detach() while ensuring that no process can
> wait in between. It's done thanks to a new injection_points_wakeup_detach()
> function that is holding the spinlock during the whole duration.
>
> - If the walsender is waiting on the injection point and that the logical slot
> is conflicting, then the walsender process is killed and so it is not able to
> "empty" it's injection slot. So the next injection_wait() should reuse this slot
> (instead of using an empty one). injection_wait() has been modified that way
> in 0001.
>
> With 0001 in place, then we can make use of an injection point in
> LogicalConfirmReceivedLocation() and update 035_standby_logical_decoding.pl to
> prevent the catalog_xmin of a logical slot to advance past the conflict point.
>
> Remarks:
>
> R1. The issue still remains in v16 though (as injection points are available since
> v17).
>

This is not idle case because the test would still keep failing
intermittently on 16. I am wondering what if we start a transaction
before vacuum and do some DML in it but didn't commit that xact till
the active_slot test is finished then even the extra logging of
xl_running_xacts shouldn't advance xmin during decoding. This is
because reorder buffer may point to the xmin before vacuum. See
following code:

SnapBuildProcessRunningXacts()
....
xmin = ReorderBufferGetOldestXmin(builder->reorder);
if (xmin == InvalidTransactionId)
xmin = running->oldestRunningXid;
elog(DEBUG3, "xmin: %u, xmax: %u, oldest running: %u, oldest xmin: %u",
builder->xmin, builder->xmax, running->oldestRunningXid, xmin);
LogicalIncreaseXminForSlot(lsn, xmin);
...

Note that I have not tested this case, so I could be wrong. But if
possible, we should try to find some solution which could be
backpatched to 16 as well.

--
With Regards,
Amit Kapila.

Hi,

On Wed, Mar 19, 2025 at 12:12:19PM +0530, Amit Kapila wrote:
> On Mon, Feb 10, 2025 at 8:12 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Please find attached a patch to $SUBJECT.
> >
> > In rare circumstances (and on slow machines) it is possible that a xl_running_xacts
> > is emitted and that the catalog_xmin of a logical slot on the standby advances
> > past the conflict point. In that case, no conflict is reported and the test
> > fails. It has been observed several times and the last discussion can be found
> > in [1].
> >
> 

Thanks for looking at it!

> Is my understanding correct that bgwriter on primary node has created
> a  xl_running_xacts, then that record is replicated to standby, and
> while decoding it (xl_running_xacts) on standby via active_slot, we
> advanced the catalog_xmin of active_slot? If this happens then the
> replay of vacuum record on standby won't be able to invalidate the
> active slot, right?

Yes, that's also my understanding. It's also easy to "simulate" by adding
a checkpoint on the primary and a long enough sleep after we launched our sql in 
wait_until_vacuum_can_remove().

> So, if the above is correct, the reason for generating extra
> xl_running_xacts on primary is Vacuum followed by Insert on primary
> via below part of test:
> $node_primary->safe_psql(
> 'testdb', qq[VACUUM $vac_option verbose $to_vac;
>   INSERT INTO flush_wal DEFAULT VALUES;]);

I'm not sure, I think a xl_running_xacts could also be generated (for example by
the checkpointer) before the vacuum (should the system be slow enough).

> > Remarks:
> >
> > R1. The issue still remains in v16 though (as injection points are available since
> > v17).
> >
> 
> This is not idle case because the test would still keep failing
> intermittently on 16.

I do agree.

> I am wondering what if we start a transaction
> before vacuum and do some DML in it but didn't commit that xact till
> the active_slot test is finished then even the extra logging of
> xl_running_xacts shouldn't advance xmin during decoding.

I'm not sure, as I think a xl_running_xacts could still be generated after
we execute "our sql" meaning:

"
    $node_primary->safe_psql('testdb', qq[$sql]);
"

and before we launch the new DML. In that case I guess the issue could still
happen.

OTOH If we create the new DML "before" we launch "our sql" then the test
would also fail for both active and inactive slots because that would not
invalidate any slots.

I did observe the above with the attached changes (just changing the PREPARE
TRANSACTION location).

> we should try to find some solution which could be
> backpatched to 16 as well.

I agree, but I'm not sure it's doable as it looks to me that we should prevent
the catalog xmin to advance to advance past the conflict point while still
generating a conflict point. Will try to give it another thought.

Regards,

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

Dear Bertrand,

I'm also working on the thread to resolve the random failure.

> Yes, that's also my understanding. It's also easy to "simulate" by adding
> a checkpoint on the primary and a long enough sleep after we launched our sql in
> wait_until_vacuum_can_remove().

Thanks for letting me know. For me, it could be reporoduced only the sleep().

> > So, if the above is correct, the reason for generating extra
> > xl_running_xacts on primary is Vacuum followed by Insert on primary
> > via below part of test:
> > $node_primary->safe_psql(
> > 'testdb', qq[VACUUM $vac_option verbose $to_vac;
> >   INSERT INTO flush_wal DEFAULT VALUES;]);
> 
> I'm not sure, I think a xl_running_xacts could also be generated (for example by
> the checkpointer) before the vacuum (should the system be slow enough).

I think you are right. When I added `CHECKPOINT` and sleep after the user SQLs,
I got the below ordering. This meant that RUNNING_XACTS are generated before the
prune triggered by the vacuum.
```
...
lsn: 0/04025218, prev 0/040251A0, desc: RUNNING_XACTS nextXid 766 latestCompletedXid 765 oldestRunningXid 766
...
lsn: 0/04028FD0, prev 0/04026FB0, desc: PRUNE_ON_ACCESS snapshotConflictHorizon: 765,...
...
```

> I'm not sure, as I think a xl_running_xacts could still be generated after
> we execute "our sql" meaning:
> 
> "
>     $node_primary->safe_psql('testdb', qq[$sql]);
> "
> 
> and before we launch the new DML. In that case I guess the issue could still
> happen.
> 
> OTOH If we create the new DML "before" we launch "our sql" then the test
> would also fail for both active and inactive slots because that would not
> invalidate any slots.
> 
> I did observe the above with the attached changes (just changing the PREPARE
> TRANSACTION location).

I've also tried the idea with the living transaction via background_psql(),
but I got the same result. The test could fail when RUNNING_XACTS record was
generated before the transaction starts.

> I agree, but I'm not sure it's doable as it looks to me that we should prevent
> the catalog xmin to advance to advance past the conflict point while still
> generating a conflict point. Will try to give it another thought.

One primitive idea for me was to stop the walsender/pg_recvlogical process for a while.
SIGSTOP signal for pg_recvlogical may do the idea, but ISTM it could not be on windows.
See 019_replslot_limit.pl.

Best regards,
Hayato Kuroda
FUJITSU LIMITED 

Hi Kuroda-san,

On Fri, Mar 21, 2025 at 12:28:10PM +0000, Hayato Kuroda (Fujitsu) wrote:
> I'm also working on the thread to resolve the random failure.

Thanks for looking at it!

> I've also tried the idea with the living transaction via background_psql(),
> but I got the same result. The test could fail when RUNNING_XACTS record was
> generated before the transaction starts.

and thanks for testing and confirming too.

> SIGSTOP signal for pg_recvlogical may do the idea,

Yeah, but would we be "really" testing an "active" slot?

At the end we want to produce an invalidation that may or not happen on a real
environment. The corner case is in the test, not an issue of the feature to
fix.

So, I'm not sure I like the idea that much, but thinking out loud: I wonder if
we could bypass the "active" slot checks in 16 and 17 and use injection points as 
proposed as of 18 (as we need the injection points changes proposed in 0001
up-thread). Thoughts?

Regards,

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

Dear Bertrand,

> > SIGSTOP signal for pg_recvlogical may do the idea,
>
> Yeah, but would we be "really" testing an "active" slot?

Yeah, this is also a debatable point.

> At the end we want to produce an invalidation that may or not happen on a real
> environment. The corner case is in the test, not an issue of the feature to
> fix.

I also think this is the test-issue, not the codebase.

> So, I'm not sure I like the idea that much, but thinking out loud: I wonder if
> we could bypass the "active" slot checks in 16 and 17 and use injection points as
> proposed as of 18 (as we need the injection points changes proposed in 0001
> up-thread). Thoughts?

I do not have other idea neither. I checked your patch set could solve the issue.

Comments for the patch:
I'm not sure whether new API is really needed. Isn't it enough to use both
injection_points_wakeup() and injection_points_detach()? This approach does not
require bumping the version, and can be backported to PG17.

Also, another check whether the extension can be installed for the node is required.
Please see 041_checkpoint_at_promote.pl.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Kuroda-san,

On Mon, Mar 24, 2025 at 04:54:21AM +0000, Hayato Kuroda (Fujitsu) wrote:
> > So, I'm not sure I like the idea that much, but thinking out loud: I wonder if
> > we could bypass the "active" slot checks in 16 and 17 and use injection points as
> > proposed as of 18 (as we need the injection points changes proposed in 0001
> > up-thread). Thoughts?
> 
> I do not have other idea neither. I checked your patch set could solve the issue.

Thanks for looking at it!

> Comments for the patch:
> I'm not sure whether new API is really needed. Isn't it enough to use both
> injection_points_wakeup() and injection_points_detach()?

I think that the proposed changes are needed as they fix 2 issues that I hit
while working on 0002:

1. ensure that no process can wait in between wakeup() and detach().

2. If the walsender is waiting on the injection point and the logical slot
is conflicting, then the walsender process is killed and so it is not able to
"empty" it's injection slot. So the next injection_wait() should reuse this slot
(instead of using an empty one).

> Also, another check whether the extension can be installed for the node is required.
> Please see 041_checkpoint_at_promote.pl.

Indeed I can see the "# Check if the extension injection_points is available, as
it may be possible that this script is run with installcheck, where the module
would not be installed by default", in 041_checkpoint_at_promote.pl.

Thanks! I think that makes sense and will add it in the proposed patch (early
next week).

Regards,

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

On Fri, Mar 21, 2025 at 9:48 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> So, I'm not sure I like the idea that much, but thinking out loud: I wonder if
> we could bypass the "active" slot checks in 16 and 17 and use injection points as
> proposed as of 18 (as we need the injection points changes proposed in 0001
> up-thread). Thoughts?
>

The key point is that snapshotConflictHorizon should always be greater
than or equal to oldestRunningXid for this test to pass. The challenge
is that vacuum LOGs the safest xid to be removed as
snapshotConflictHorizon, which I think will always be either one or
more lesser than oldestRunningXid. So, we can't make it pass unless we
ensure there is no running_xact record gets logged after the last
successful transaction (in this case SQL passed to function
wait_until_vacuum_can_remove) and the till the vacuum is replayed on
the standby. I see even check_for_invalidation('pruning_', $logstart,
'with on-access pruning'); failed  [1].

Seeing all these failures, I wonder whether we can reliably test
active slots apart from wal_level change test (aka Scenario 6:
incorrect wal_level on primary.). Sure, we can try by having some
injection point kind of tests, but is it really worth because, anyway
the active slots won't get invalidated in the scenarios for row
removal we are testing in this case. The other possibility is to add a
developer-level debug_disable_running_xact GUC to test this and
similar cases, or can't we have an injection point to control logging
this WAL record? I have seen the need to control logging running_xact
record in other cases as well.

[1] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-03-19%2007%3A08%3A16

--
With Regards,
Amit Kapila.

Dear Amit, Bertrand,

> Seeing all these failures, I wonder whether we can reliably test
> active slots apart from wal_level change test (aka Scenario 6:
> incorrect wal_level on primary.). Sure, we can try by having some
> injection point kind of tests, but is it really worth because, anyway
> the active slots won't get invalidated in the scenarios for row
> removal we are testing in this case. The other possibility is to add a
> developer-level debug_disable_running_xact GUC to test this and
> similar cases, or can't we have an injection point to control logging
> this WAL record? I have seen the need to control logging running_xact
> record in other cases as well.

Based on the idea which controls generating RUNNING_XACTS, I prototyped a patch.
When the instance is attached the new injection point, all processes would skip
logging the record. This does not need to extend injection_point module.
I tested with reproducer and passed on my env.
Sadly IS_INJECTION_POINT_ATTACHED() was introduced for PG18 so that the patch
could not backport for PG17 as-is.

How do you feel?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Amit, Bertrand,

> Seeing all these failures, I wonder whether we can reliably test
> active slots apart from wal_level change test (aka Scenario 6:
> incorrect wal_level on primary.).

Hmm, agreed. We do not have good solution to stabilize tests, at least for now.
I've created a patch for PG16 which avoids using active slots in scenario 1, 2, 3,
and 5 like attached. Other tests still use active slots:

* Scenario 6 invalidate slots due to the incorrect wal_level, so it retained.
* 'behaves_ok_' testcase, scenario 4 and 'Test standby promotion...' testcase
  won't invalidate slots, so they retained.
* 'DROP DATABASE should drops...' invalidates slots, but it does not related with
  xmin horizon, so it retained.

The patch aimed only PG16, but can be created for PG17 as well, if needed.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Mar 26, 2025 at 1:17 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > Seeing all these failures, I wonder whether we can reliably test
> > active slots apart from wal_level change test (aka Scenario 6:
> > incorrect wal_level on primary.). Sure, we can try by having some
> > injection point kind of tests, but is it really worth because, anyway
> > the active slots won't get invalidated in the scenarios for row
> > removal we are testing in this case. The other possibility is to add a
> > developer-level debug_disable_running_xact GUC to test this and
> > similar cases, or can't we have an injection point to control logging
> > this WAL record? I have seen the need to control logging running_xact
> > record in other cases as well.
>
> Based on the idea which controls generating RUNNING_XACTS, I prototyped a patch.
> When the instance is attached the new injection point, all processes would skip
> logging the record. This does not need to extend injection_point module.
>

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.

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.

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?

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.

> 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.

--
With Regards,
Amit Kapila.

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

Hi Kuroda-san and Amit,

On Fri, Mar 28, 2025 at 09:02:29AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Amit,
> 
> > 
> > Right, I think this is a better idea.

I like it too and the bonus point is that this injection point can be used
in more tests (more use cases).

A few comments:

==== About v2-0001-Stabilize

=== 1

s/to avoid the seeing a xl_running_xacts/to avoid seeing a xl_running_xacts/?

=== 2 (Nit)

/* For testing slot invalidation due to the conflict */

Not sure "due to the conflict" is needed.

==== About PG17-v2-0001

=== 3

The commit message still mentions injection point.

=== 4

-# Note that pg_current_snapshot() is used to get the horizon.  It does
-# not generate a Transaction/COMMIT WAL record, decreasing the risk of
-# seeing a xl_running_xacts that would advance an active replication slot's
-# catalog_xmin.  Advancing the active replication slot's catalog_xmin
-# would break some tests that expect the active slot to conflict with
-# the catalog xmin horizon.

I'd be tempted to not remove this comment but reword it a bit instead. Something
like?

# Note that pg_current_snapshot() is used to get the horizon.  It does
# not generate a Transaction/COMMIT WAL record, decreasing the risk of
# seeing a xl_running_xacts that would advance an active replication slot's
# catalog_xmin.  Advancing the active replication slot's catalog_xmin
# would break some tests that expect the active slot to conflict with
# the catalog xmin horizon. We ensure that active replication slots are not
# created for tests that might produce this race condition though. 

=== 5

The invalidation checks for active slots are kept for the wal_level case. Also
the active slots are still created to test that logical decoding on the standby 
behaves correctly, when no conflict is expected and for the promotion.

The above makes sense to me.

=== 6 (Nit)

In drop_logical_slots(), s/needs_active_slot/drop_active_slot/?

=== 7 (Nit)

In check_slots_conflict_reason(), s/needs_active_slot/checks_active_slot/?

==== About PG16-v2-0001

Same as for PG17-v2-0001.

Regards,

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

Dear Bertrand,

> s/to avoid the seeing a xl_running_xacts/to avoid seeing a xl_running_xacts/?

Fixed.

>
> === 2 (Nit)
>
> /* For testing slot invalidation due to the conflict */
>
> Not sure "due to the conflict" is needed.
>

OK, removed.

> ==== About PG17-v2-0001
>
> === 3
>
> The commit message still mentions injection point.

Oh, removed.

> === 4
>
> -# Note that pg_current_snapshot() is used to get the horizon.  It does
> -# not generate a Transaction/COMMIT WAL record, decreasing the risk of
> -# seeing a xl_running_xacts that would advance an active replication slot's
> -# catalog_xmin.  Advancing the active replication slot's catalog_xmin
> -# would break some tests that expect the active slot to conflict with
> -# the catalog xmin horizon.
>
> I'd be tempted to not remove this comment but reword it a bit instead. Something
> like?
>
> # Note that pg_current_snapshot() is used to get the horizon.  It does
> # not generate a Transaction/COMMIT WAL record, decreasing the risk of
> # seeing a xl_running_xacts that would advance an active replication slot's
> # catalog_xmin.  Advancing the active replication slot's catalog_xmin
> # would break some tests that expect the active slot to conflict with
> # the catalog xmin horizon. We ensure that active replication slots are not
> # created for tests that might produce this race condition though.

Added.

> === 6 (Nit)
>
> In drop_logical_slots(), s/needs_active_slot/drop_active_slot/?

Fixed.

> === 7 (Nit)
>
> In check_slots_conflict_reason(), s/needs_active_slot/checks_active_slot/?

Fixed.

> ==== About PG16-v2-0001
>
> Same as for PG17-v2-0001.

I followed all needed changes.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Kuroda-san,

On Tue, Apr 01, 2025 at 01:22:49AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Bertrand,
> 

Thanks for the updated patch!

> > s/to avoid the seeing a xl_running_xacts/to avoid seeing a xl_running_xacts/?
> 
> Fixed.

hmm, not sure as I still can see:

+# Note that injection_point is used to avoid the seeing the xl_running_xacts

=== 1

+                * XXX What value should we return here? Originally this returns the
+                * inserted location of RUNNING_XACT record. Based on that, here
+                * returns the latest insert location for now.
+                */
+               return GetInsertRecPtr();

Looking at the LogStandbySnapshot() that are using the output lsn, i.e:

pg_log_standby_snapshot()
BackgroundWriterMain()
ReplicationSlotReserveWal()

It looks ok to me to use GetInsertRecPtr().

But if we "really" want to produce a "new" WAL record, what about using
LogLogicalMessage()? It could also be used for debugging purpose. Bonus point:
it does not need wal_level to be set to logical. Thoughts?

Regards,

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

On Tue, Apr 1, 2025 at 2:02 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi Kuroda-san,
>
> On Tue, Apr 01, 2025 at 01:22:49AM +0000, Hayato Kuroda (Fujitsu) wrote:
> > Dear Bertrand,
> >
>
> Thanks for the updated patch!
>
> > > s/to avoid the seeing a xl_running_xacts/to avoid seeing a xl_running_xacts/?
> >
> > Fixed.
>
> hmm, not sure as I still can see:
>
> +# Note that injection_point is used to avoid the seeing the xl_running_xacts
>
> === 1
>
> +                * XXX What value should we return here? Originally this returns the
> +                * inserted location of RUNNING_XACT record. Based on that, here
> +                * returns the latest insert location for now.
> +                */
> +               return GetInsertRecPtr();
>
> Looking at the LogStandbySnapshot() that are using the output lsn, i.e:
>
> pg_log_standby_snapshot()
> BackgroundWriterMain()
> ReplicationSlotReserveWal()
>
> It looks ok to me to use GetInsertRecPtr().
>

+1.

> But if we "really" want to produce a "new" WAL record, what about using
> LogLogicalMessage()?
>

We are using injection points for testing purposes, which means the
caller is aware of skipping the running_xacts record during the test
run. So, there doesn't seem to be any reason to do anything extra.

--
With Regards,
Amit Kapila.

On Tue, Apr 1, 2025 at 6:53 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>

With respect to 0001, can't this problem happen for the following case as well?
# Recovery conflict: Invalidate conflicting slots, including in-use slots
# Scenario 5: conflict due to on-access pruning.

You have not added any injection point for the above case. Isn't it
possible that if running_xact record is logged concurrently to the
pruning record, it should move the active slot on standby, and the
same failure should occur in this case as well?

--
With Regards,
Amit Kapila.

Dear Bertrand,

> > > s/to avoid the seeing a xl_running_xacts/to avoid seeing a xl_running_xacts/?
> >
> > Fixed.

Sorry, I misunderstood your comment and wrongly fixed. I will address in next version.

> === 1
>
> +                * XXX What value should we return here? Originally this
> returns the
> +                * inserted location of RUNNING_XACT record. Based on that,
> here
> +                * returns the latest insert location for now.
> +                */
> +               return GetInsertRecPtr();
>
> Looking at the LogStandbySnapshot() that are using the output lsn, i.e:
>
> pg_log_standby_snapshot()
> BackgroundWriterMain()
> ReplicationSlotReserveWal()
>
> It looks ok to me to use GetInsertRecPtr().
>
> But if we "really" want to produce a "new" WAL record, what about using
> LogLogicalMessage()? It could also be used for debugging purpose. Bonus point:
> it does not need wal_level to be set to logical. Thoughts?

Right. Similarly, an SQL function pg_logical_emit_message() is sometimes used for
the testing purpose, advance_wal() and emit_wal( in Cluster.pm. Even so, we have
not found the use-case yet, thus I want to retain now and will update based on
the future needs.

I'll investigate another point [1] and then will post new version.

[1]: https://www.postgresql.org/message-id/CAA4eK1%2Bx5-eOn5%2BMW6FiUjB_1bBCH8jCCARC1uMrx6erZ3J73w%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

On Tue, Apr 01, 2025 at 04:55:06PM +0530, Amit Kapila wrote:
> On Tue, Apr 1, 2025 at 2:02 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> 
> > But if we "really" want to produce a "new" WAL record, what about using
> > LogLogicalMessage()?
> >
> 
> We are using injection points for testing purposes, which means the
> caller is aware of skipping the running_xacts record during the test
> run. So, there doesn't seem to be any reason to do anything extra.

Agree, the idea was to provide extra debugging info for the tests. We can just
keep it in mind should we have a need for.

Regards,

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

Dear Amit, Bertrand,

> You have not added any injection point for the above case. Isn't it
> possible that if running_xact record is logged concurrently to the
> pruning record, it should move the active slot on standby, and the
> same failure should occur in this case as well?

I considered that the timing failure can happen. Reproducer:

```
 $node_primary->safe_psql('testdb', qq[UPDATE prun SET s = 'D';]);
+$node_primary->safe_psql('testdb', 'CHECKPOINT');
+sleep(20);
 $node_primary->safe_psql('testdb', qq[UPDATE prun SET s = 'E';]);
```

And here is my theory...

Firstly, a new table was created with smaller fill factor. Then, after doing UPDATE
three times, the page became full. At fourth UPDATE command (let's say txn4),
the page pruning was done by the backend process and PRUNE_ON_ACCESS was generated.
It requested standbys to discard tuples before the third UPDATE (say txn3),
thus the slot could be invalidated.
However, if a RUNNING_XACTS record is generated between txn3 and txn4, the
oldestRunningXact would be same xid as txn4, and the catalog_xmin of the standby
slot would be advanced till that. Upcoming PRUNE_ON_ACCESS points the txn3 so that
slot invalidation won't happen in this case.

Based on the fact, I've updated to use injection_points for scenario 5. Of course,
PG16/17 patches won't use the active slot for that scenario.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Kuroda-san,

On Wed, Apr 02, 2025 at 07:16:25AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Amit, Bertrand,
> 
> > You have not added any injection point for the above case. Isn't it
> > possible that if running_xact record is logged concurrently to the
> > pruning record, it should move the active slot on standby, and the
> > same failure should occur in this case as well?
> 
> I considered that the timing failure can happen. Reproducer:
> 
> ```
>  $node_primary->safe_psql('testdb', qq[UPDATE prun SET s = 'D';]);
> +$node_primary->safe_psql('testdb', 'CHECKPOINT');
> +sleep(20);
>  $node_primary->safe_psql('testdb', qq[UPDATE prun SET s = 'E';]);
> ```

Yeah, I was going to provide the exact same reproducer and then saw your email.

> Based on the fact, I've updated to use injection_points for scenario 5. Of course,
> PG16/17 patches won't use the active slot for that scenario.

Thanks for the updated patch!

As far v4-0001:

=== 1

+# would advance an active replication slot's catalog_xmin

s/would/could/? I mean the system also needs to be "slow" enough (so the
sleep() in the reproducer)

=== 2

+# Injection_point is used to avoid seeing an xl_running_xacts even here. In
+# scenario 5, we verify the case that the backend process detects the page has
+# enough tuples; thus, page pruning happens. If the record is generated just
+# before doing on-pruning, the catalog_xmin of the active slot would be
+# updated; hence, the conflict would not occur.

Not sure we need to explain what scenario 5 does here, but that does not hurt
if you feel the need.

Also maybe mention the last update in the comment and add some nuance (like
proposed in === 1), something like?

"
# Injection_point is used to avoid seeing a xl_running_xacts here. Indeed,
# if it is generated between the last 2 updates then the catalog_xmin of the active
# slot could be updated; hence, the conflict could not occur.
"

Apart from that the tests looks good to me and all the problematic scenarios
covered.

As far PG17-v4-0001:

=== 3

-# seeing a xl_running_xacts that would advance an active replication slot's
+# seeing the xl_running_xacts that would advance an active replication slot's

why?

=== 4

It looks like that check_slots_conflict_reason() is not called with checks_active_slot
as argument.

=== 5

I think that we could remove the need for the drop_active_slot parameter in
drop_logical_slots() and just check if an active slot exists (and if so drop
it). That said I'm not sure it's worth to go that far for backpatching.

As far PG16-v4:

=== 6

Same as === 3 and === 5 (=== 4 does not apply as check_slots_conflict_reason()
does not exist).

Regards,

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

On Wed, Apr 2, 2025 at 2:06 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi Kuroda-san,
>
> On Wed, Apr 02, 2025 at 07:16:25AM +0000, Hayato Kuroda (Fujitsu) wrote:
>
> As far v4-0001:
>
> === 1
>
> +# would advance an active replication slot's catalog_xmin
>
> s/would/could/? I mean the system also needs to be "slow" enough (so the
> sleep() in the reproducer)
>
> === 2
>
> +# Injection_point is used to avoid seeing an xl_running_xacts even here. In
> +# scenario 5, we verify the case that the backend process detects the page has
> +# enough tuples; thus, page pruning happens. If the record is generated just
> +# before doing on-pruning, the catalog_xmin of the active slot would be
> +# updated; hence, the conflict would not occur.
>
> Not sure we need to explain what scenario 5 does here, but that does not hurt
> if you feel the need.
>
> Also maybe mention the last update in the comment and add some nuance (like
> proposed in === 1), something like?
>
> "
> # Injection_point is used to avoid seeing a xl_running_xacts here. Indeed,
> # if it is generated between the last 2 updates then the catalog_xmin of the active
> # slot could be updated; hence, the conflict could not occur.
> "
>

I have changed it based on your suggestions and made a few other
changes in the comments. Please see attached.

*
+  if (IS_INJECTION_POINT_ATTACHED("log-running-xacts"))

It is better to name the injection point as skip-log-running-xacts as
that will be appropriate based on its usage.

--
With Regards,
Amit Kapila.

On Wed, Apr 2, 2025 at 2:06 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
>
> As far PG17-v4-0001:
>
>
> === 4
>
> It looks like that check_slots_conflict_reason() is not called with checks_active_slot
> as argument.
>
> === 5
>
> I think that we could remove the need for the drop_active_slot parameter in
> drop_logical_slots() and just check if an active slot exists (and if so drop
> it). That said I'm not sure it's worth to go that far for backpatching.
>

The other idea to simplify the changes for backbranches:
sub reactive_slots_change_hfs_and_wait_for_xmins
{
...
+  my ($slot_prefix, $hsf, $invalidated, $needs_active_slot) = @_;

  # create the logical slots
-  create_logical_slots($node_standby, $slot_prefix);
+  create_logical_slots($node_standby, $slot_prefix, $needs_active_slot);

...
+  if ($needs_active_slot)
+  {
+    $handle =
+      make_slot_active($node_standby, $slot_prefix, 1, \$stdout, \$stderr);
+  }

What if this function doesn't take input parameter needs_active_slot
and rather removes the call to make_slot_active? We will call
make_slot_active only at the required places. This should make the
changes much less because after that, we don't need to make changes
related to drop and create. Sure, in some cases, we will test two
inactive slots instead of one, but I guess that would be the price to
keep the tests simple and more like HEAD.

--
With Regards,
Amit Kapila.

Hi,

On Wed, Apr 02, 2025 at 03:04:07PM +0530, Amit Kapila wrote:
> I have changed it based on your suggestions and made a few other
> changes in the comments. Please see attached.

Thanks!

> *
> +  if (IS_INJECTION_POINT_ATTACHED("log-running-xacts"))
> 
> It is better to name the injection point as skip-log-running-xacts as
> that will be appropriate based on its usage.

Agree.

+# Note that the injection_point avoids seeing a xl_running_xacts that could
and
+# Injection_point avoids seeing an xl_running_xacts even here. This is required

s/an xl_running_xacts/a xl_running_xacts/? in the second one? Also I'm not sure
"even here" is needed.

Apart from the above that LGTM.

Regards,

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

Dear Amit, Bertrand,

> The other idea to simplify the changes for backbranches:
> sub reactive_slots_change_hfs_and_wait_for_xmins
> {
> ...
> +  my ($slot_prefix, $hsf, $invalidated, $needs_active_slot) = @_;
> 
>   # create the logical slots
> -  create_logical_slots($node_standby, $slot_prefix);
> +  create_logical_slots($node_standby, $slot_prefix, $needs_active_slot);
> 
> ...
> +  if ($needs_active_slot)
> +  {
> +    $handle =
> +      make_slot_active($node_standby, $slot_prefix, 1, \$stdout, \$stderr);
> +  }
> 
> What if this function doesn't take input parameter needs_active_slot
> and rather removes the call to make_slot_active? We will call
> make_slot_active only at the required places. This should make the
> changes much less because after that, we don't need to make changes
> related to drop and create. Sure, in some cases, we will test two
> inactive slots instead of one, but I guess that would be the price to
> keep the tests simple and more like HEAD.

Actually, I could not decide which one is better, so let me share both drafts.
V5-PG17-1 uses the previous approach, and v5-PG17-2 uses new proposed one.
Bertrand, which one do you like?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Apr 02, 2025 at 12:13:52PM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Amit, Bertrand,
> 
> > The other idea to simplify the changes for backbranches:
> > sub reactive_slots_change_hfs_and_wait_for_xmins
> > {
> > ...
> > +  my ($slot_prefix, $hsf, $invalidated, $needs_active_slot) = @_;
> > 
> >   # create the logical slots
> > -  create_logical_slots($node_standby, $slot_prefix);
> > +  create_logical_slots($node_standby, $slot_prefix, $needs_active_slot);
> > 
> > ...
> > +  if ($needs_active_slot)
> > +  {
> > +    $handle =
> > +      make_slot_active($node_standby, $slot_prefix, 1, \$stdout, \$stderr);
> > +  }
> > 
> > What if this function doesn't take input parameter needs_active_slot
> > and rather removes the call to make_slot_active? We will call
> > make_slot_active only at the required places. This should make the
> > changes much less because after that, we don't need to make changes
> > related to drop and create. Sure, in some cases, we will test two
> > inactive slots instead of one, but I guess that would be the price to
> > keep the tests simple and more like HEAD.
> 
> Actually, I could not decide which one is better, so let me share both drafts.

Thanks!

> V5-PG17-1 uses the previous approach, and v5-PG17-2 uses new proposed one.
> Bertrand, which one do you like?

I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
keep the slots active and only avoid doing the checks for them (they are invalidated
that's fine, they are not that's fine too).

Also I think that we should change this part:

"
 # Verify that invalidated logical slots do not lead to retaining WAL.
@@ -602,7 +610,7 @@ check_slots_conflict_reason('vacuum_full_', 'rows_removed');
 my $restart_lsn = $node_standby->safe_psql(
        'postgres',
        "SELECT restart_lsn FROM pg_replication_slots
-               WHERE slot_name = 'vacuum_full_activeslot' AND conflicting;"
+               WHERE slot_name = 'vacuum_full_inactiveslot' AND conflicting;"
 );

" to be on the safe side of thing.

What do you think of the attached (to apply on top of v5-PG17-2)?

Regards,

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

On Wed, Apr 2, 2025 at 8:30 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Wed, Apr 02, 2025 at 12:13:52PM +0000, Hayato Kuroda (Fujitsu) wrote:
> > Dear Amit, Bertrand,
> >
> > > The other idea to simplify the changes for backbranches:
> > > sub reactive_slots_change_hfs_and_wait_for_xmins
> > > {
> > > ...
> > > +  my ($slot_prefix, $hsf, $invalidated, $needs_active_slot) = @_;
> > >
> > > # create the logical slots
> > > -  create_logical_slots($node_standby, $slot_prefix);
> > > +  create_logical_slots($node_standby, $slot_prefix, $needs_active_slot);
> > >
> > > ...
> > > +  if ($needs_active_slot)
> > > +  {
> > > +    $handle =
> > > +      make_slot_active($node_standby, $slot_prefix, 1, \$stdout, \$stderr);
> > > +  }
> > >
> > > What if this function doesn't take input parameter needs_active_slot
> > > and rather removes the call to make_slot_active? We will call
> > > make_slot_active only at the required places. This should make the
> > > changes much less because after that, we don't need to make changes
> > > related to drop and create. Sure, in some cases, we will test two
> > > inactive slots instead of one, but I guess that would be the price to
> > > keep the tests simple and more like HEAD.
> >
> > Actually, I could not decide which one is better, so let me share both drafts.
>
> Thanks!
>
> > V5-PG17-1 uses the previous approach, and v5-PG17-2 uses new proposed one.
> > Bertrand, which one do you like?
>
> I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
> keep the slots active and only avoid doing the checks for them (they are invalidated
> that's fine, they are not that's fine too).
>

I don't mind doing that, but there is no benefit in making slots
active unless we can validate them. And we will end up adding some
more checks, as in function check_slots_conflict_reason without any
advantage. I feel Kuroda-San's second patch is simple, and we have
fewer chances to make mistakes and easy to maintain in the future as
well.

--
With Regards,
Amit Kapila.

Dear Bertrand, Amit,

> > I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
> > keep the slots active and only avoid doing the checks for them (they are
> invalidated
> > that's fine, they are not that's fine too).
> >
> 
> I don't mind doing that, but there is no benefit in making slots
> active unless we can validate them. And we will end up adding some
> more checks, as in function check_slots_conflict_reason without any
> advantage. I feel Kuroda-San's second patch is simple, and we have
> fewer chances to make mistakes and easy to maintain in the future as
> well.

I have concerns for Bertrand's patch that it could introduce another timing
issue. E.g., if the activated slots are not invalidated, dropping slots is keep
being activated so the dropping might be fail. I did not reproduce this but
something like this can happen if we activate slots.

Attached patch has a conclusion of these discussions, slots are created but
it seldomly be activated.

Naming of patches are bit different, but please ignore...

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Apr 3, 2025 at 11:04 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Bertrand, Amit,
>
> > > I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
> > > keep the slots active and only avoid doing the checks for them (they are
> > invalidated
> > > that's fine, they are not that's fine too).
> > >
> >
> > I don't mind doing that, but there is no benefit in making slots
> > active unless we can validate them. And we will end up adding some
> > more checks, as in function check_slots_conflict_reason without any
> > advantage. I feel Kuroda-San's second patch is simple, and we have
> > fewer chances to make mistakes and easy to maintain in the future as
> > well.
>
> I have concerns for Bertrand's patch that it could introduce another timing
> issue. E.g., if the activated slots are not invalidated, dropping slots is keep
> being activated so the dropping might be fail. I did not reproduce this but
> something like this can happen if we activate slots.
>
> Attached patch has a conclusion of these discussions, slots are created but
> it seldomly be activated.
>
> Naming of patches are bit different, but please ignore...
>

 Isn't patch 0001-Fix-invalid-referring-of-hash-ref-for-replication-sl
unrelated to this thread? Or am, I missing something?

--
With Regards,
Amit Kapila.

>  Isn't patch 0001-Fix-invalid-referring-of-hash-ref-for-replication-sl
> unrelated to this thread? Or am, I missing something?

I did attach wrongly, PSA correct set. Sorry for inconvenience.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi,

On Thu, Apr 03, 2025 at 05:34:10AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Bertrand, Amit,
> 
> > > I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
> > > keep the slots active and only avoid doing the checks for them (they are
> > invalidated
> > > that's fine, they are not that's fine too).
> > >
> > 
> > I don't mind doing that, but there is no benefit in making slots
> > active unless we can validate them. And we will end up adding some
> > more checks, as in function check_slots_conflict_reason without any
> > advantage.

I think that there is advantage. The pros are:

- the test would be closer to HEAD from a behavioural point of view
- it's very rare to hit the corner cases: so the test would behave the same
as on HEAD most of the time (and when it does not that would not hurt as the
checks are nor done)
- Kuroda-San's patch removes "or die "replication slot stats of vacuum_full_activeslot not updated"
while keeping the slot active is able to keep it (should the slot being invalidated
or not). But more on that in the comment === 1 below.

> I feel Kuroda-San's second patch is simple, and we have
> > fewer chances to make mistakes and easy to maintain in the future as
> > well.

Yeah maybe but the price to pay is to discard the pros above. That said, I'm also
fine with Kuroda-San's patch if both of you feel that it's better.

> I have concerns for Bertrand's patch that it could introduce another timing
> issue. E.g., if the activated slots are not invalidated, dropping slots is keep
> being activated so the dropping might be fail.

Yeah, but the drop is done with "$node_standby->psql" so that the test does not
produce an error. It would produce an error should we use "$node_standby->safe_psql"
instead.

> I did not reproduce this but
> something like this can happen if we activate slots.

You can see it that way (+ reproducer.txt):

"
+       my $bdt = $node_standby->safe_psql('postgres', qq[SELECT * from pg_replication_slots]);
+       note "BDT: $bdt";
+
        $node_standby->psql('postgres',
                qq[SELECT pg_drop_replication_slot('$inactive_slot')]);
"

You'd see the slot being active and the "$node_standby->psql" not reporting
any error.

> Attached patch has a conclusion of these discussions, slots are created but
> it seldomly be activated.

Thanks for the patch!

=== 1

-$node_standby->poll_query_until('testdb',
-       qq[SELECT total_txns > 0 FROM pg_stat_replication_slots WHERE slot_name = 'vacuum_full_activeslot']
-) or die "replication slot stats of vacuum_full_activeslot not updated";
-
 # This should trigger the conflict
 wait_until_vacuum_can_remove(

I wonder if we could not keep this test and make the slot active for the
vacuum full case. Looking at drongo's failure in [1], there is no occurence
of "vacuum full" and that's probably linked to Andres's explanation in [2]:

"
a VACUUM FULL on pg_class is
used, which prevents logical decoding from progressing after it started (due
to the logged AEL at the start of VACFULL).
"

meaning that the active slot is invalidated even if the catalog xmin's moves
forward due to xl_running_xacts.

[1]: https://www.postgresql.org/message-id/386386.1737736935%40sss.pgh.pa.us
[2]: https://www.postgresql.org/message-id/zqypkuvtihtd2zbmwdfmcceujg4fuakrhojmjkxpp7t4udqkty%40couhenc7dsor

Regards,

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

On Thu, Apr 3, 2025 at 12:29 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Thu, Apr 03, 2025 at 05:34:10AM +0000, Hayato Kuroda (Fujitsu) wrote:
> > Dear Bertrand, Amit,
> >
> > > > I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
> > > > keep the slots active and only avoid doing the checks for them (they are
> > > invalidated
> > > > that's fine, they are not that's fine too).
> > > >
> > >
> > > I don't mind doing that, but there is no benefit in making slots
> > > active unless we can validate them. And we will end up adding some
> > > more checks, as in function check_slots_conflict_reason without any
> > > advantage.
>
> I think that there is advantage. The pros are:
>
> - the test would be closer to HEAD from a behavioural point of view
> - it's very rare to hit the corner cases: so the test would behave the same
> as on HEAD most of the time (and when it does not that would not hurt as the
> checks are nor done)
> - Kuroda-San's patch removes "or die "replication slot stats of vacuum_full_activeslot not updated"
> while keeping the slot active is able to keep it (should the slot being invalidated
> or not). But more on that in the comment === 1 below.
>
> > I feel Kuroda-San's second patch is simple, and we have
> > > fewer chances to make mistakes and easy to maintain in the future as
> > > well.
>
> Yeah maybe but the price to pay is to discard the pros above. That said, I'm also
> fine with Kuroda-San's patch if both of you feel that it's better.
>
> > I have concerns for Bertrand's patch that it could introduce another timing
> > issue. E.g., if the activated slots are not invalidated, dropping slots is keep
> > being activated so the dropping might be fail.
>
> Yeah, but the drop is done with "$node_standby->psql" so that the test does not
> produce an error. It would produce an error should we use "$node_standby->safe_psql"
> instead.
>
> > I did not reproduce this but
> > something like this can happen if we activate slots.
>
> You can see it that way (+ reproducer.txt):
>
> "
> +       my $bdt = $node_standby->safe_psql('postgres', qq[SELECT * from pg_replication_slots]);
> +       note "BDT: $bdt";
> +
>         $node_standby->psql('postgres',
>                 qq[SELECT pg_drop_replication_slot('$inactive_slot')]);
> "
>
> You'd see the slot being active and the "$node_standby->psql" not reporting
> any error.
>

Hmm, but adding some additional smarts also makes this test less easy
to backpatch. I see your points related to the benefits, but I still
mildly prefer to go with the lesser changes approach for backbranches
patch. Normally, we don't enhance backbranches code without making
equivalent changes in HEAD, so adding some new bugs only in
backbranches has a lesser chance.

--
With Regards,
Amit Kapila.

Dear Bertrand,

> I wonder if we could not keep this test and make the slot active for the
> vacuum full case. Looking at drongo's failure in [1], there is no occurence
> of "vacuum full" and that's probably linked to Andres's explanation in [2]:
>
> "
> a VACUUM FULL on pg_class is
> used, which prevents logical decoding from progressing after it started (due
> to the logged AEL at the start of VACFULL).
> "
>

I had been debugging and found the case that VACUUM FULL also has a timing issue.
This means the we cannot keep the testcase.

PSA the reproducer for PG17. IIUC this can happen even in PG16.
I considered what happened here;

1. Run a CHECKPOINT and wait sometime in wait_until_vacuum_can_remove().
   This ensures that RUNNING_XACTS record can be generated and catalog_xmin can
   be advanced after the user SQLs.
2. Assuming that another RUNNING_XACTS record is generated *WHILE* doing a VACUUM
   FULL. This can be done by the periodic checkpoint or the reproducer.
3. Logical walsender detects the RUNNING_XACTS record.
   Note that this must be done before startup tries to invalidate slot.
4. In sometime the walsender receives the ack and advance the catalog_xmin.
   Note again that this must be done before startup tries to invalidate slot.
5. Startup process detects the PRUNE_ON_ACCESS record and tries to invalidate the
   slot. However, the catalog_xmin has been advanced so that the invalidation
   cannot be done.

Analysis
========
While analyzing this workload, I found that VACUUM FULL can generate four
PRUNE_ON_ACCESS records. More especially, first two records are generated while
clustering the table, others are done while updating pg_database.datfrozenxid.
Interestingly, latter records are genareted after the transaction is finished;
the VACUUM FULL command itselfs ends up the txn once (in vacuum_rel) and then
continue working on. Without the delay in testcode, the first PRUNE record leads
the invalidation the slot, and with the delay fourth PRUNE leads it. Per my
analysis, snapshotConflictHorizon is the xid which first PRUNE records exist.

Based on the fact, I considered that catalog_xmin can be advanced till the between
(non-)transactional PRUNE records. RequestCheckpoint() is added to generate the
RUNNING_XACTS in-between them.

Very thanks Amit for supporting me off-list for reproducing the issue.

Best regards,
Hayato Kuroda
Fujitsu LIMITED

Hi Kuroda-san,

On Mon, Apr 07, 2025 at 06:15:13AM +0000, Hayato Kuroda (Fujitsu) wrote:
> I had been debugging and found the case that VACUUM FULL also has a timing issue.
> This means the we cannot keep the testcase.
> 
> PSA the reproducer for PG17. IIUC this can happen even in PG16.
> I considered what happened here;
> 
> 1. Run a CHECKPOINT and wait sometime in wait_until_vacuum_can_remove().
>    This ensures that RUNNING_XACTS record can be generated and catalog_xmin can
>    be advanced after the user SQLs.
> 2. Assuming that another RUNNING_XACTS record is generated *WHILE* doing a VACUUM
>    FULL. This can be done by the periodic checkpoint or the reproducer.
> 3. Logical walsender detects the RUNNING_XACTS record.
>    Note that this must be done before startup tries to invalidate slot.
> 4. In sometime the walsender receives the ack and advance the catalog_xmin.
>    Note again that this must be done before startup tries to invalidate slot.
> 5. Startup process detects the PRUNE_ON_ACCESS record and tries to invalidate the
>    slot. However, the catalog_xmin has been advanced so that the invalidation
>    cannot be done.

Thanks for the testing and explanation! I did apply your repro and I'm able to
see the test failing (with an active slot). The scenario is more unlikely
to happen (as compare to the non vacuum full cases) and that's why it was not
visible in drongo's reports in [1]. So yeah, let's do as you suggested and do
not make the slot active for the vacuum full case too.

[1]: https://www.postgresql.org/message-id/386386.1737736935@sss.pgh.pa.us

Regards,

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

On Thu, Apr 3, 2025 at 3:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Apr 3, 2025 at 12:29 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > On Thu, Apr 03, 2025 at 05:34:10AM +0000, Hayato Kuroda (Fujitsu) wrote:
> > > Dear Bertrand, Amit,
> > >
> > > > > I do prefer v5-PG17-2 as it is "closer" to HEAD. That said, I think that we should
> > > > > keep the slots active and only avoid doing the checks for them (they are
> > > > invalidated
> > > > > that's fine, they are not that's fine too).
> > > > >
> > > >
> > > > I don't mind doing that, but there is no benefit in making slots
> > > > active unless we can validate them. And we will end up adding some
> > > > more checks, as in function check_slots_conflict_reason without any
> > > > advantage.
> >
> > I think that there is advantage. The pros are:
> >
> > - the test would be closer to HEAD from a behavioural point of view
> > - it's very rare to hit the corner cases: so the test would behave the same
> > as on HEAD most of the time (and when it does not that would not hurt as the
> > checks are nor done)
> > - Kuroda-San's patch removes "or die "replication slot stats of vacuum_full_activeslot not updated"
> > while keeping the slot active is able to keep it (should the slot being invalidated
> > or not). But more on that in the comment === 1 below.
> >
> > > I feel Kuroda-San's second patch is simple, and we have
> > > > fewer chances to make mistakes and easy to maintain in the future as
> > > > well.
> >
> > Yeah maybe but the price to pay is to discard the pros above. That said, I'm also
> > fine with Kuroda-San's patch if both of you feel that it's better.
> >
> > > I have concerns for Bertrand's patch that it could introduce another timing
> > > issue. E.g., if the activated slots are not invalidated, dropping slots is keep
> > > being activated so the dropping might be fail.
> >
> > Yeah, but the drop is done with "$node_standby->psql" so that the test does not
> > produce an error. It would produce an error should we use "$node_standby->safe_psql"
> > instead.
> >
> > > I did not reproduce this but
> > > something like this can happen if we activate slots.
> >
> > You can see it that way (+ reproducer.txt):
> >
> > "
> > +       my $bdt = $node_standby->safe_psql('postgres', qq[SELECT * from pg_replication_slots]);
> > +       note "BDT: $bdt";
> > +
> >         $node_standby->psql('postgres',
> >                 qq[SELECT pg_drop_replication_slot('$inactive_slot')]);
> > "
> >
> > You'd see the slot being active and the "$node_standby->psql" not reporting
> > any error.
> >
>
> Hmm, but adding some additional smarts also makes this test less easy
> to backpatch. I see your points related to the benefits, but I still
> mildly prefer to go with the lesser changes approach for backbranches
> patch. Normally, we don't enhance backbranches code without making
> equivalent changes in HEAD, so adding some new bugs only in
> backbranches has a lesser chance.
>

Bertrand, do you agree with the fewer changes approach (where active
slots won't be tested) for backbranches? I think now that we have
established that the vacuum full test is also prone to failure due to
race condition in the test, this is the only remaining open point.

--
With Regards,
Amit Kapila.

Hi,

On Mon, Apr 07, 2025 at 03:16:07PM +0530, Amit Kapila wrote:
> On Thu, Apr 3, 2025 at 3:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Hmm, but adding some additional smarts also makes this test less easy
> > to backpatch. I see your points related to the benefits, but I still
> > mildly prefer to go with the lesser changes approach for backbranches
> > patch. Normally, we don't enhance backbranches code without making
> > equivalent changes in HEAD, so adding some new bugs only in
> > backbranches has a lesser chance.
> >
> 
> Bertrand, do you agree with the fewer changes approach (where active
> slots won't be tested) for backbranches? I think now that we have
> established that the vacuum full test is also prone to failure due to
> race condition in the test, this is the only remaining open point.

Yeah that's all good on my side, let's keep it that way and don't make the slot
active.

Regards,

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

On Thu, Apr 3, 2025 at 11:29 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>

I have changed quite a few comments and commit message for the PG17
patch in the attached. Can you update PG16 patch based on this and
also use the same commit message as used in attached for all the three
patches?

--
With Regards,
Amit Kapila.

Dear Amit,

> 
> I have changed quite a few comments and commit message for the PG17
> patch in the attached. Can you update PG16 patch based on this and
> also use the same commit message as used in attached for all the three
> patches?

Your patch looks good to me and it could pass on my env. PSA patches for PG16.
Patch for PG17 is not changed, just renamed.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Apr 08, 2025 at 02:00:35AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Your patch looks good to me and it could pass on my env. PSA patches for PG16.
> Patch for PG17 is not changed, just renamed.

@@ -1287,6 +1288,17 @@ LogStandbySnapshot(void)

     Assert(XLogStandbyInfoActive());

+#ifdef USE_INJECTION_POINTS
+    if (IS_INJECTION_POINT_ATTACHED("skip-log-running-xacts"))
+    {
+        /*
+         * This record could move slot's xmin forward during decoding, leading
+         * to unpredictable results, so skip it when requested by the test.
+         */
+        return GetInsertRecPtr();
+    }
+#endif

I have unfortunately not been able to pay much attention to this
thread, but using an injection point as a trick to disable the
generation of these random standby snapshot records is an interesting
approach to stabilize the test, and it should make it faster as well.
Nice.
--
Michael

Hi,

On Tue, Apr 08, 2025 at 02:13:20PM +0900, Michael Paquier wrote:
> On Tue, Apr 08, 2025 at 02:00:35AM +0000, Hayato Kuroda (Fujitsu) wrote:
> > Your patch looks good to me and it could pass on my env. PSA patches for PG16.
> > Patch for PG17 is not changed, just renamed.
> 
> @@ -1287,6 +1288,17 @@ LogStandbySnapshot(void)
>  
>      Assert(XLogStandbyInfoActive());
>  
> +#ifdef USE_INJECTION_POINTS
> +    if (IS_INJECTION_POINT_ATTACHED("skip-log-running-xacts"))
> +    {
> +        /*
> +         * This record could move slot's xmin forward during decoding, leading
> +         * to unpredictable results, so skip it when requested by the test.
> +         */
> +        return GetInsertRecPtr();
> +    }
> +#endif
> 
> I have unfortunately not been able to pay much attention to this
> thread, but using an injection point as a trick to disable the
> generation of these random standby snapshot records is an interesting
> approach to stabilize the test, and it should make it faster as well.
> Nice.

Yeah. That said I still think it could be useful to implement what has been
proposed in v1-0001 in [1] i.e:

- A new injection_points_wakeup_detach() function that is holding the spinlock
during the whole duration to ensure that no process can wait in between the
wakeup and the detach.

- injection_wait() should try to reuse an existing slot (if any) before trying
to use an empty one.

This is not needed here anymore (as we're using another injection point that the
one initially prooposed) but I'll open a dedicated thread for that for 19 when
the timing will be appropriate.

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

Regards,

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

Hi,

On 2025-04-08 02:00:35 +0000, Hayato Kuroda (Fujitsu) wrote:
> > I have changed quite a few comments and commit message for the PG17
> > patch in the attached. Can you update PG16 patch based on this and
> > also use the same commit message as used in attached for all the three
> > patches?
> 
> Your patch looks good to me and it could pass on my env. PSA patches for PG16.
> Patch for PG17 is not changed, just renamed.

Thanks all for working together to for fix this.  These test failures were
really rather painful!


Now we just need to fix the issue that causes random CI failures on windows
and the one that causes similar, but different, random failures on macos...

Greetings,

Andres Freund

On Tue, Apr 08, 2025 at 06:19:02AM +0000, Bertrand Drouvot wrote:
> - A new injection_points_wakeup_detach() function that is holding the spinlock
> during the whole duration to ensure that no process can wait in between the
> wakeup and the detach.

That would not a correct spinlock use.  injection_points_detach() and
injection_points_wakeup_internal() do much more actions than what we
can internally do while holding a spinlock, including both
Postgres-specific calls as well as system calls.  strcmp() and
strlcpy() are still OK-ish, even as system calls, as they work
directly on the strings given in input.
--
Michael

Hi,

On Tue, Apr 08, 2025 at 03:27:41PM +0900, Michael Paquier wrote:
> On Tue, Apr 08, 2025 at 06:19:02AM +0000, Bertrand Drouvot wrote:
> > - A new injection_points_wakeup_detach() function that is holding the spinlock
> > during the whole duration to ensure that no process can wait in between the
> > wakeup and the detach.
> 
> That would not a correct spinlock use.  injection_points_detach() and
> injection_points_wakeup_internal() do much more actions than what we
> can internally do while holding a spinlock,

Fully agree. Will need to find another way to prevent a process to wait between the
wakeup and the detach. I'll open a dedicated thread.

Regards,

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

On Tue, Apr 08, 2025 at 06:42:53AM +0000, Bertrand Drouvot wrote:
> Fully agree. Will need to find another way to prevent a process to wait between the
> wakeup and the detach. I'll open a dedicated thread.

By the way, there is a small thing that's itching me a bit about the
change done in LogStandbySnapshot() for 105b2cb33617.  Could it be
useful for debugging to add a elog(DEBUG1) with the LSN returned by
GetInsertRecPtr() when taking the short path?  We don't have any
visibility when the shortcut path is taken, which seems annoying in
the long term if we use the injection point skip-log-running-xacts for
other tests, and I suspect that there will be some as the standby
snapshots can be really annoying in tests where we want a predictible
set of WAL records when wal_level is "replica" or "logical".
--
Michael

Hi,

On Wed, Apr 09, 2025 at 12:03:06PM +0900, Michael Paquier wrote:
> On Tue, Apr 08, 2025 at 06:42:53AM +0000, Bertrand Drouvot wrote:
> > Fully agree. Will need to find another way to prevent a process to wait between the
> > wakeup and the detach. I'll open a dedicated thread.
> 
> By the way, there is a small thing that's itching me a bit about the
> change done in LogStandbySnapshot() for 105b2cb33617.  Could it be
> useful for debugging to add a elog(DEBUG1) with the LSN returned by
> GetInsertRecPtr() when taking the short path?  We don't have any
> visibility when the shortcut path is taken, which seems annoying in
> the long term if we use the injection point skip-log-running-xacts for
> other tests, and I suspect that there will be some as the standby
> snapshots can be really annoying in tests where we want a predictible
> set of WAL records when wal_level is "replica" or "logical".

Yeah, I also think that would be good to have some way to debug this. That's
why I did propose to generate a WAL record by making use of LogLogicalMessage()
instead of GetInsertRecPtr() (see [1]). We could generate "bypassing xl_running_xacts"
or such and bonus point that would advance the record ptr. 

Adding an elog(DEBUG1) could make sense too.

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

Regards,

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

On Wed, Apr 9, 2025 at 11:24 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Wed, Apr 09, 2025 at 12:03:06PM +0900, Michael Paquier wrote:
> > On Tue, Apr 08, 2025 at 06:42:53AM +0000, Bertrand Drouvot wrote:
> > > Fully agree. Will need to find another way to prevent a process to wait between the
> > > wakeup and the detach. I'll open a dedicated thread.
> >
> > By the way, there is a small thing that's itching me a bit about the
> > change done in LogStandbySnapshot() for 105b2cb33617.  Could it be
> > useful for debugging to add a elog(DEBUG1) with the LSN returned by
> > GetInsertRecPtr() when taking the short path?  We don't have any
> > visibility when the shortcut path is taken, which seems annoying in
> > the long term if we use the injection point skip-log-running-xacts for
> > other tests, and I suspect that there will be some as the standby
> > snapshots can be really annoying in tests where we want a predictible
> > set of WAL records when wal_level is "replica" or "logical".
>
> Yeah, I also think that would be good to have some way to debug this.
>

I can't think of a good reason to have this DEBUG1 as there is no
predictability of it getting generated even with tests using an
injection point. OTOH, I don't have any objections to it if you would
like to proceed with this.

--
With Regards,
Amit Kapila.

On Wed, Apr 09, 2025 at 12:07:31PM +0530, Amit Kapila wrote:
> On Wed, Apr 9, 2025 at 11:24 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> I can't think of a good reason to have this DEBUG1 as there is no
> predictability of it getting generated even with tests using an
> injection point. OTOH, I don't have any objections to it if you would
> like to proceed with this.

The non-predictability of the event is my reason, as it can be useful
to know this information when grabbing for specific patterns in the
logs between failed and successful run differences.  In short, I'd
like to think that we are OK here, still this information is free to
have and it could be useful if we still have problems.  A custom
message WAL record is overdoing in it a bit, IMO, an elog() with the
LSN returned should be enough.
--
Michael