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

From: Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com>
To: Peter Smith <smithpb2250(at)gmail(dot)com>
Cc: Melanie Plageman <melanieplageman(at)gmail(dot)com>, "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, shveta malik <shveta(dot)malik(at)gmail(dot)com>
Subject: Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Date: 2023-06-23 13:50:24
Message-ID: CAGPVpCQs6X-HtvEVrh0DdUDX1hi0PWF9pX7+1KsgzrmURxAOYg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Peter,

Thanks for your reviews. I tried to apply most of them. I just have
some comments below for some of them.

Peter Smith <smithpb2250(at)gmail(dot)com>, 14 Haz 2023 Çar, 08:45 tarihinde
şunu yazdı:
>
> 9. process_syncing_tables_for_sync
>
> @@ -378,7 +387,13 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn)
> */
> replorigin_drop_by_name(originname, true, false);
>
> - finish_sync_worker();
> + /*
> + * 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);
>
> 9a.
> I did not quite follow the logic. It says "Sync worker is cleaned at
> this point", but who is doing that? -- more details are needed. But,
> why not just call clean_sync_worker() right here like it use to call
> finish_sync_worker?

I agree that these explanations at places where the worker decides to
not continue with the current table were confusing. Even the name of
ready_to_reuse was misleading. I renamed it and tried to improve
comments in such places.
Can you please check if those make more sense now?

> ======
> src/backend/replication/logical/worker.c
>
> 10. General -- run_tablesync_worker, TablesyncWorkerMain
>
> IMO these functions would be more appropriately reside in the
> tablesync.c instead of the (common) worker.c. Was there some reason
> why they cannot be put there?

I'm not really against moving those functions to tablesync.c. But
what's not clear to me is worker.c. Is it the places to put common
functions for all log. rep. workers? Then, what about apply worker?
Should we consider a separate file for apply worker too?

Thanks,
--
Melih Mutlu
Microsoft

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Hayato Kuroda (Fujitsu) 2023-06-23 14:25:13 [PGdocs] fix description for handling pf non-ASCII characters
Previous Message Melih Mutlu 2023-06-23 13:39:57 Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication