On Fri, Jan 3, 2025 at 12:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I have one comment on the 0001 patch:
>
> + /*
> + * The changes made by this and later transactions are still
> non-removable
> + * to allow for the detection of update_deleted conflicts when applying
> + * changes in this logical replication worker.
> + *
> + * Note that this info cannot directly protect dead tuples from being
> + * prematurely frozen or removed. The logical replication launcher
> + * asynchronously collects this info to determine whether to advance the
> + * xmin value of the replication slot.
> + *
> + * Therefore, FullTransactionId that includes both the
> transaction ID and
> + * its epoch is used here instead of a single Transaction ID. This is
> + * critical because without considering the epoch, the transaction ID
> + * alone may appear as if it is in the future due to transaction ID
> + * wraparound.
> + */
> + FullTransactionId oldest_nonremovable_xid;
>
> The last paragraph of the comment mentions that we need to use
> FullTransactionId to properly compare XIDs even after the XID
> wraparound happens. But once we set the oldest-nonremovable-xid it
> prevents XIDs from being wraparound, no? I mean that workers'
> oldest-nonremovable-xid values and slot's non-removal-xid (i.e., its
> xmin) are never away from more than 2^31 XIDs.
>
I also think that the slot's non-removal-xid should ensure that we
never allow xid to advance to a level where it can cause a wraparound
for the oldest-nonremovable-xid value stored in each worker because
the slot's value is the minimum of all workers. Now, if both of us are
missing something then it is probably better to write some more
detailed comments as to how this can happen.
Along the same lines, I was thinking whether
RetainConflictInfoData->last_phase_at should be FullTransactionId but
I think that is correct because we can't stop wraparound from
happening on remote_node, right?
--
With Regards,
Amit Kapila.
