Horton

Re: Conflict detection for update_deleted in logical replication

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, vignesh C <vignesh21@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-07-03T10:51:31Z
Lists: pgsql-hackers 
On Thu, Jul 3, 2025 at 10:57 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jul 3, 2025 at 10:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Jul 3, 2025 at 10:26 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > >
> > > You changes related to write barrier LGTM, however I have question
> > > regarding below change, IIUC, in logical replication
> > > MyReplicationSlot->effective_xmin should be the xmin value which has
> > > been flushed to the disk, but here we are just setting "data.xmin =
> > > new_xmin;" and marking the slot dirty so I believe its not been yet
> > > flushed to the disk right?
> > >
> >
> > Yes, because this is a physical slot and we need to follow
> > PhysicalConfirmReceivedLocation()/PhysicalReplicationSlotNewXmin().
> > The patch has kept a comment in advance_conflict_slot_xmin() as to why
> > it is okay not to flush the slot immediately.
>
> Oh right, I forgot its physical slot.  I think we are good, thanks.
>

BTW, I wanted to clarify one more point related to this part of the
patch. One difference between PhysicalReplicationSlotNewXmin() and
advance_conflict_slot_xmin() is that the former updates both
catalog_xmin and xmin for the slot, but later updates only the slot's
xmin. Can we see any reason to update both in our case? For example,
there is one case where the caller of
ProcArrayGetReplicationSlotXmin() expects that the catalog_xmin must
be set when required, but as far as I can see, it is required only
when logical slots are present, so we should be okay with that case.

The other case to consider is vacuum calling
GetOldestNonRemovableTransactionId() to get the cutoff xid to remove
deleted rows. This returns the xmin horizon based on the type of table
(user table, catalog table, etc.). Now, in this case,
ComputeXidHorizons() will first set data xmin for
catalog_oldest_nonremovable xid, and then if slot_catalog_xmin is
smaller, it uses that value. So, for this computation as well, setting
just slot's xmin in advance_conflict_slot_xmin() should be sufficient,
as we will anyway set both xmin and catalog_xmin to the same values.

By this theory, it doesn't matter whether we set catalog_xmin for
physical slots or not till we are setting the slot's xmin. IIUC,
catalog_xmin is required to be set for logical slots because during
logical decoding, we access only catalog tables, so we need to protect
those, and the catalog_xmin value is used for that.

Thoughts?

-- 
With Regards,
Amit Kapila.

Commits

  1. Fix intermittent BF failures in 035_conflicts.

  2. Resume conflict-relevant data retention automatically.

  3. Fix intermittent test failure introduced in 6456c6e2c4.

  4. Fix Coverity issue reported in commit a850be2fe.

  5. Add test to prevent premature removal of conflict-relevant data.

  6. Post-commit review fixes for 228c370868.

  7. Add max_retention_duration option to subscriptions.

  8. Detect and report update_deleted conflicts.

  9. Preserve conflict-relevant data during logical replication.

  10. Integrate FullTransactionIds deeper into two-phase code

  11. Improve checks for GUC recovery_target_timeline

  12. Prevent excessive delays before launching new logrep workers.

  13. Keep WAL segments by slot's last saved restart LSN

  14. Rework some code handling pg_subscription data in psql and pg_dump

  15. Use generateClonedIndexStmt to propagate CREATE INDEX to partitions.