Re: Synchronizing slots from primary to standby

On Wed, Aug 23, 2023 at 3:38 PM shveta malik <shveta.malik@gmail.com> wrote:
>
I have reviewed the v12-0002 patch and I have some comments. I see the
latest version posted sometime back and if any of this comment is
already fixed in this version then feel free to ignore that.

In general, code still needs a lot more comments to make it readable
and in some places, code formatting is also not as per PG standard so
that needs to be improved.
There are some other specific comments as listed below

1.
@@ -925,7 +936,7 @@ ApplyLauncherRegister(void)
  memset(&bgw, 0, sizeof(bgw));
  bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
  BGWORKER_BACKEND_DATABASE_CONNECTION;
- bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
+ bgw.bgw_start_time = BgWorkerStart_ConsistentState;

What is the reason for this change, can you add some comments?

2.
ApplyLauncherShmemInit(void)
 {
  bool found;
+ bool foundSlotSync;

Is there any specific reason to launch the sync worker from the
logical launcher instead of making this independent?
I mean in the future if we plan to sync physical slots as well then it
wouldn't be an expandable design.

3.
+ /*
+ * Remember the old dbids before we stop and cleanup this worker
+ * as these will be needed in order to relaunch the worker.
+ */
+ copied_dbcnt = worker->dbcount;
+ copied_dbids = (Oid *)palloc0(worker->dbcount * sizeof(Oid));
+
+ for (i = 0; i < worker->dbcount; i++)
+ copied_dbids[i] = worker->dbids[i];

probably we can just memcpy the memory containing the dbids.

4.
+ /*
+ * If worker is being reused, and there is vacancy in dbids array,
+ * just update dbids array and dbcount and we are done.
+ * But if dbids array is exhausted, stop the worker, reallocate
+ * dbids in dsm, relaunch the worker with same set of dbs as earlier
+ * plus the new db.
+ */

Why do we need to relaunch the worker, can't we just use dsa_pointer
to expand the shared memory whenever required?

5.

+static bool
+WaitForSlotSyncWorkerAttach(SlotSyncWorker *worker,
+    uint16 generation,
+    BackgroundWorkerHandle *handle)

this function is an exact duplicate of WaitForReplicationWorkerAttach
except for the LWlock, why don't we use the same function by passing
the LWLock as a parameter

6.
+/*
+ * Attach Slot-sync worker to worker-slot assigned by launcher.
+ */
+void
+slotsync_worker_attach(int slot)

this is also very similar to the logicalrep_worker_attach function.

Please check other similar functions and reuse them wherever possible

Also, why this function is not registering the cleanup function on shmmem_exit?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com