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

Hi,

I rebased the patch and addressed the following reviews.

Peter Smith <smithpb2250@gmail.com>, 24 May 2023 Çar, 05:59 tarihinde
şunu yazdı:
> Here are a few other random things noticed while looking at patch 0001:
>
> 1. Commit message
>
> 1a. typo /sequantially/sequentially/
>
> 1b. Saying "killed" and "killing" seemed a bit extreme and implies
> somebody else is killing the process. But I think mostly tablesync is
> just ending by a normal proc exit, so maybe reword this a bit.
>

Fixed the type and reworded a bit.

>
> 2. It seemed odd that some -- clearly tablesync specific -- functions
> are in the worker.c instead of in tablesync.c.
>
> 2a. e.g. clean_sync_worker
>
> 2b. e.g. sync_worker_exit
>

Honestly, the distinction between worker.c and tablesync.c is not that
clear to me. Both seem like they're doing some work for tablesync and
apply.
But yes, you're right. Those functions fit better to tablesync.c. Moved them.

>
> 3. process_syncing_tables_for_sync
>
> + /*
> + * Sync worker is cleaned at this point. It's ready to sync next table,
> + * if needed.
> + */
> + SpinLockAcquire(&MyLogicalRepWorker->relmutex);
> + MyLogicalRepWorker->ready_to_reuse = true;
>   SpinLockRelease(&MyLogicalRepWorker->relmutex);
> + }
> +
> + SpinLockRelease(&MyLogicalRepWorker->relmutex);
>
> Isn't there a double release of that mutex happening there?

Fixed.

Thanks,
--
Melih Mutlu
Microsoft