Re: Follow-up review items for update_deleted

On Fri, Sep 4, 2026 at 3:27 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Friday, September 4, 2026 3:27 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> > Thanks for the updated patch.
> >
> > I was testing/reviewing the retention stop-resume path when two dbs (say
> > db1 and db2) are involved. I see a race condition between the launcher and
> > the worker that leads to the same assert.
> > When a sub resumes retention on db2, update_retention_status(true) sets
> > subretentionactive = true and calls ApplyLauncherWakeup() before the
> > worker restarts itself through apply_worker_exit().
> >
> > Now if the launcher runs its next cycle while the old worker's entry is still
> > in_use(say we hold it just before apply_worker_exit), then in that cycle,
> > launcher will -
> > - add the db2 to retained_dbids.
> > - fire the reset because db2 is not yet in the set.
> > - Finds w != NULL (still old worker), so
> > compute_min_nonremovable_xid() reads the "InvalidTransactionId" left by
> > the earlier stop and returns without contributing to the result.
> >
> > Now the situation is that update_conflict_slot_xmin() advances the slot to the
> > horizon of db1 (which can be way ahead of db2's horizon).
> > When eventually a new worker starts for db2's subscription with a valid
> > oldest_xmin based on db2's horizon, in the next launcher cycle, since db2 is
> > already in the retained_dbids list, it will not fire the reset again. The slot.xmin
> > stays advanced, and when the db2 worker reaches get_candidate_xid() with
> > its old xmin, it hits the same assertion.
> >
> > TRAP: failed Assert("TransactionIdPrecedesOrEquals(MyLogicalRepWorker-
> > ...
> >
> > Attached is a TAP test that reproduces this issue using an injection point. The
> > patch applies on top of v2-0001.
>
> Thanks for reporting this. I think we should skip updating the slot.xmin in the
> cycle where the old worker hasn't stopped but retention has been resumed. Here's
> the updated patch that fixes this. To make it cleaner, I also refactored the
> logic slightly to centralize the decision of whether to update the slot.xmin.
>
>
> On Thursday, September 3, 2026 5:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > 4
> > >
> > > With track_commit_timestamp off and retain_dead_tuples on,
> > > FindDeletedTupleInLocalRel() returns false unconditionally and silently,
> > while
> > > dead tuples are still retained.
> > >
> > > This is not considered as a bug, as the documentation clearly states that
> > > update_deleted requires track_commit_timestamp to be enabled, and we
> > > emit a WARNING when retain_dead_tuples is enabled without it. If needed, we
> > > could improve this by stopping retention when track_commit_timestamp is
> > > disabled, but disabling track_commit_timestamp while retain_dead_tuples is enabled
> > > seems like an extremely unlikely user behavior. So would be better to wait for user
> > > feedback before taking any action.
> > >
> > > 5
> > >
> > > max_retention_duration does not work for a disabled subscription or a
> > > keep-failing worker, meaning retention cannot be stopped using this option
> > > in those cases.
> > >
> > > This is documented behavior (and user can disable the retain_dead_tuples
> > > manually):
> > >
> > > > This option is effective only when retain_dead_tuples is enabled and the apply
> > > > worker associated with the subscription is active.
> >
> > Both 4 and 5 are documented but will it be better to provide this
> > information explicitly, say via Logging when the conflcit_slot is not
> > getting advanced due to any of these reasons?
>
> We can do that. It seems to me we could make this improvement for PG20 at this
> stage. For PG19, I think we can improve the documentation to make it a bit
> clearer instead. Amit and I discussed this off-list, and See 0002 for the patch
> Amit shared to improve the doc.
>

I am still reviewing patch001; for patch 002 I have one comment. I
feel the following information should be added as a CAUTION or under
the existing WARNING section similar to how we explain the
data-acuumulation fact under CAUTION section for for
retain_dead_tuples GUC.

max_retention_duration
If the
+ subscription is disabled, or its apply worker is not running, the
+ retention duration is not evaluated and the information for conflict
+ detection continues to accumulate regardless of this setting. In that
+ case retention can only be stopped by disabling
+ <literal>retain_dead_tuples</literal>.
------------

Also, shall we change it slightly to:

Note that the retention duration is not evaluated while the
subscription is disabled or its apply worker is not running. Thus, the
information retained for conflict detection will continue to
accumulate regardless of this setting until the subscription is
enabled or its apply worker resumes. To prevent excessive
accumulation, consider disabling <literal>retain_dead_tuples</literal>
if the subscription will be inactive for an extended period.

thanks
Shveta