Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication - Mailing list pgsql-hackers

From Melih Mutlu
Subject Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Date
Msg-id CAGPVpCTvALKEXe0=N-+iMmVxVQ-+P8KZ_1qQ1KsSSZ-V9wJ5hw@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
List pgsql-hackers
Hi,

PFA updated patches. Rebased 0003 with minor changes. Addressed Peter's reviews for 0001 and 0002 with some small comments below.

Peter Smith <smithpb2250@gmail.com>, 10 Tem 2023 Pzt, 10:09 tarihinde şunu yazdı:
6. LogicalRepApplyLoop

+ /*
+ * apply_dispatch() may have gone into apply_handle_commit()
+ * which can call process_syncing_tables_for_sync.
+ *
+ * process_syncing_tables_for_sync decides whether the sync of
+ * the current table is completed. If it is completed,
+ * streaming must be already ended. So, we can break the loop.
+ */
+ if (MyLogicalRepWorker->is_sync_completed)
+ {
+ endofstream = true;
+ break;
+ }
+

and

+ /*
+ * If is_sync_completed is true, this means that the tablesync
+ * worker is done with synchronization. Streaming has already been
+ * ended by process_syncing_tables_for_sync. We should move to the
+ * next table if needed, or exit.
+ */
+ if (MyLogicalRepWorker->is_sync_completed)
+ endofstream = true;

~

Instead of those code fragments above assigning 'endofstream' as a
side-effect, would it be the same (but tidier) to just modify the
other "breaking" condition below:

BEFORE:
/* Check if we need to exit the streaming loop. */
if (endofstream)
break;

AFTER:
/* Check if we need to exit the streaming loop. */
if (endofstream || MyLogicalRepWorker->is_sync_completed)
break;

First place you mentioned also breaks the infinite loop. Such an if statement is needed there with or without endofstream assignment.

I think if there is a flag to break a loop, using that flag to indicate that we should exit the loop seems more appropriate to me. I see that it would be a bit tidier without endofstream = true lines, but I feel like it would also be less readable. 

I don't have a strong opinion though. I'm just keeping them as they are for now, but I can change them if you disagree.
 

10b.
All the other tablesync-related fields of this struct are named as
relXXX, so I wonder if is better for this to follow the same pattern.
e.g. 'relsync_completed'

Aren't those start with rel because they're related to the relation that the tablesync worker is syncing? is_sync_completed is not a relation specific field. I'm okay with changing the name but feel like relsync_completed would be misleading. 

Thanks,
--
Melih Mutlu
Microsoft
Attachment

pgsql-hackers by date:

Previous
From: Isaac Morland
Date:
Subject: Re: Looking for context around which event triggers are permitted
Next
From: Garrett Thornburg
Date:
Subject: Re: Looking for context around which event triggers are permitted