On Fri, Dec 6, 2024 at 1:28 PM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> On Thursday, December 5, 2024 6:00 PM Amit Kapila <amit.kapil...@gmail.com>
> wrote:
> >
> >
> > A few more comments:
> > 1.
> > +static void
> > +wait_for_local_flush(RetainConflictInfoData *data)
> > {
> > ...
> > +
> > + data->phase = RCI_GET_CANDIDATE_XID;
> > +
> > + maybe_advance_nonremovable_xid(data);
> > +}
> >
> > Isn't it better to reset all the fields of data before the next round of
> > GET_CANDIDATE_XID phase? If we do that then we don't need to reset
> > data->remote_lsn = InvalidXLogRecPtr; and data->last_phase_at =
> > InvalidFullTransactionId; individually in request_publisher_status() and
> > get_candidate_xid() respectively. Also, it looks clean and logical to me
> > unless I
> > am missing something.
>
> The remote_lsn was used to determine whether a status is received, so was
> reset
> each time in request_publisher_status. To make it more straightforward, I
> added
> a new function parameter 'status_received', which would be set to true when
> calling maybe_advance_nonremovable_xid() on receving the status. After this
> change, there is no need to reset the remote_lsn.
>
As part of the above comment, I had asked for three things (a) avoid
setting data->remote_lsn = InvalidXLogRecPtr; in
request_publisher_status(); (b) avoid setting data->last_phase_at
=InvalidFullTransactionId; in get_candidate_xid(); (c) reset data in
wait_for_local_flush() after wait is over. You only did (a) in the
patch and didn't mention anything about (b) or (c). Is that
intentional? If so, what is the reason?
*
+static bool
+can_advance_nonremovable_xid(RetainConflictInfoData *data)
+{
+
Isn't it better to make this an inline function as it contains just one check?
*
+ /*
+ * The non-removable transaction ID for a subscription is centrally
+ * managed by the main apply worker.
+ */
+ if (!am_leader_apply_worker())
I have tried to improve this comment in the attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/worker.c
b/src/backend/replication/logical/worker.c
index a75a3691dc..b1b77e4a1e 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4078,8 +4078,9 @@ maybe_advance_nonremovable_xid(RetainConflictInfoData
*data,
bool status_received)
{
/*
- * The non-removable transaction ID for a subscription is centrally
- * managed by the main apply worker.
+ * It is sufficient to manage non-removable transaction ID for a
+ * subscription by the main apply worker to detect update_deleted
conflict
+ * even for table sync or parallel apply workers.
*/
if (!am_leader_apply_worker())
return;
