Re: Conflict detection for update_deleted in logical replication

On Thu, Jul 3, 2025 at 4:21 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jul 3, 2025 at 10:57 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Thu, Jul 3, 2025 at 10:43 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Jul 3, 2025 at 10:26 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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?

IMHO the purpose of these 2 functions are different, I think the
PhysicalReplicationSlotNewXmin() update the xmin in response to
hot_standby_feedback and the purpose of that is to avoid removing
anything on primary until it is no longer required by the standby so
that we do not create conflict or query cancellation. So it has to
consider the data required by active queries, physical/logical
replication slots on standby etc. Whereas the purpose of
advance_conflict_slot_xmin() to prevent the tuple from removing on the
subscriber which might required for the conflict detection on this
node, the other aspect of not removing the tuple which is required for
the logical/physical replication slots on this node is already taken
care by other slots. So I think this is a very specific purpose build
slot which just has one specific task, so I feel we are good with what
we have.

--
Regards,
Dilip Kumar
Google