Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication

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(at)gmail(dot)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