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

On Thu, Jun 1, 2023 6:54 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> 
> Hi,
> 
> I rebased the patch and addressed the following reviews.
> 

Thanks for updating the patch. Here are some comments on 0001 patch.

1.
-	ereport(LOG,
-			(errmsg("logical replication table synchronization worker for subscription \"%s\", table \"%s\" has finished",
-					MySubscription->name,
-					get_rel_name(MyLogicalRepWorker->relid))));

Could we move this to somewhere else instead of removing it?

2.
+	if (!OidIsValid(originid))
+		originid = replorigin_create(originname);
+	replorigin_session_setup(originid, 0);
+	replorigin_session_origin = originid;
+	*origin_startpos = replorigin_session_get_progress(false);
+	CommitTransactionCommand();
+
+	/* Is the use of a password mandatory? */
+	must_use_password = MySubscription->passwordrequired &&
+		!superuser_arg(MySubscription->owner);
+	LogRepWorkerWalRcvConn = walrcv_connect(MySubscription->conninfo, true,
+											must_use_password,
+											MySubscription->name, &err);

It seems that there is a problem when refactoring.
See commit e7e7da2f8d5.

3.
+	/* Set this to false for safety, in case we're already reusing the worker */
+	MyLogicalRepWorker->ready_to_reuse = false;
+

I am not sure do we need to lock when setting it.

4.
+	/*
+	 * Allocate the origin name in long-lived context for error context
+	 * message.
+	 */
+	StartTransactionCommand();
+	ReplicationOriginNameForLogicalRep(MySubscription->oid,
+									   MyLogicalRepWorker->relid,
+									   originname,
+									   originname_size);
+	CommitTransactionCommand();

Do we need the call to StartTransactionCommand() and CommitTransactionCommand()
here? Besides, the comment here is the same as the comment atop
set_apply_error_context_origin(), do we need it?

5.
I saw a segmentation fault when debugging.

It happened when calling sync_worker_exit() called (see the code below in
LogicalRepSyncTableStart()). In the case that this is not the first table the
worker synchronizes, clean_sync_worker() has been called before (in
TablesyncWorkerMain()), and LogRepWorkerWalRcvConn has been set to NULL. Then, a
segmentation fault happened because LogRepWorkerWalRcvConn is a null pointer.

	switch (relstate)
	{
		case SUBREL_STATE_SYNCDONE:
		case SUBREL_STATE_READY:
		case SUBREL_STATE_UNKNOWN:
			sync_worker_exit();	/* doesn't return */
	}

Here is the backtrace.

#0  0x00007fc8a8ce4c95 in libpqrcv_disconnect (conn=0x0) at libpqwalreceiver.c:757
#1  0x000000000092b8c0 in clean_sync_worker () at tablesync.c:150
#2  0x000000000092b8ed in sync_worker_exit () at tablesync.c:164
#3  0x000000000092d8f6 in LogicalRepSyncTableStart (origin_startpos=0x7ffd50f30f08) at tablesync.c:1293
#4  0x0000000000934f76 in start_table_sync (origin_startpos=0x7ffd50f30f08, myslotname=0x7ffd50f30e80) at worker.c:4457
#5  0x000000000093513b in run_tablesync_worker (options=0x7ffd50f30ec0, slotname=0x0, originname=0x7ffd50f30f10 "pg_16394_16395",
    originname_size=64, origin_startpos=0x7ffd50f30f08) at worker.c:4532
#6  0x0000000000935a3a in TablesyncWorkerMain (main_arg=1) at worker.c:4853
#7  0x00000000008e97f6 in StartBackgroundWorker () at bgworker.c:864
#8  0x00000000008f350b in do_start_bgworker (rw=0x12fc1a0) at postmaster.c:5762
#9  0x00000000008f38b7 in maybe_start_bgworkers () at postmaster.c:5986
#10 0x00000000008f2975 in process_pm_pmsignal () at postmaster.c:5149
#11 0x00000000008ee98a in ServerLoop () at postmaster.c:1770
#12 0x00000000008ee3bb in PostmasterMain (argc=3, argv=0x12c4af0) at postmaster.c:1463
#13 0x00000000007b6d3a in main (argc=3, argv=0x12c4af0) at main.c:198


The steps to reproduce: 
Worker1, in TablesyncWorkerMain(), the relstate of new table to sync (obtained
by GetSubscriptionRelations) is SUBREL_STATE_INIT, and in the foreach loop,
before the following Check (it needs a breakpoint before locking),

			LWLockAcquire(LogicalRepWorkerLock, LW_EXCLUSIVE);
			if (rstate->state != SUBREL_STATE_SYNCDONE &&
				!logicalrep_worker_find(MySubscription->oid, rstate->relid, false))
			{
				/* Update worker state for the next table */
				MyLogicalRepWorker->relid = rstate->relid;
				MyLogicalRepWorker->relstate = rstate->state;
				MyLogicalRepWorker->relstate_lsn = rstate->lsn;
				LWLockRelease(LogicalRepWorkerLock);
				break;
			}
			LWLockRelease(LogicalRepWorkerLock);

let this table to be synchronized by another table sync worker (Worker2), and
Worker2 has finished before logicalrep_worker_find was called(). Then Worker1
tried to sync a table whose state is SUBREL_STATE_READY and the segmentation
fault happened.

Regards,
Shi Yu