On Thu, Jul 3, 2025 at 4:21 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Jul 3, 2025 at 10:57 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Thu, Jul 3, 2025 at 10:43 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Thu, Jul 3, 2025 at 10:26 AM Dilip Kumar <dilipbal...@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?
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
