Re: Excessive number of replication slots for 12->14 logical replication

Here is my review comment for v4-0001.

(Just a nitpick about comments)

======

1. src/backend/replication/logical/tablesync.c - process_syncing_tables_for_sync

There are really just 2 main things that are being done for the cleanup here:
- Drop the origin tracking
- Drop the tablesync slot

So, IMO the code will have more clarity by having one bigger comment
for each of those drops, instead of commenting every step separately.

e.g.

BEFORE
/*
* Cleanup the tablesync origin tracking if exists.
*/
ReplicationOriginNameForTablesync(MyLogicalRepWorker->subid,
  MyLogicalRepWorker->relid,
  originname,
  sizeof(originname));

/*
* Reset the origin session before dropping.
*
* This is required to reset the ownership of the slot
* and allow the origin to be dropped.
*/
replorigin_session_reset();

replorigin_session_origin = InvalidRepOriginId;
replorigin_session_origin_lsn = InvalidXLogRecPtr;
replorigin_session_origin_timestamp = 0;

/*
* There is a chance that the user is concurrently performing
* refresh for the subscription where we remove the table
* state and its origin and by this time the origin might be
* already removed. So passing missing_ok = true.
*/
replorigin_drop_by_name(originname, true, false);

/* Cleanup the tablesync slot. */
ReplicationSlotNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
syncslotname,
sizeof(syncslotname));

/*
* It is important to give an error if we are unable to drop the slot,
* otherwise, it won't be dropped till the corresponding subscription
* is dropped. So passing missing_ok = false.
*/
ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);


SUGGESTION

/*
* Cleanup the tablesync origin tracking.
*
* Resetting the origin session removes the ownership of the slot.
* This is needed to allow the origin to be dropped.
*
* Also, there is a chance that the user is concurrently performing
* refresh for the subscription where we remove the table
* state and its origin and by this time the origin might be
* already removed. So passing missing_ok = true.
*/
ReplicationOriginNameForTablesync(MyLogicalRepWorker->subid,
  MyLogicalRepWorker->relid,
  originname,
  sizeof(originname));
replorigin_session_reset();
replorigin_session_origin = InvalidRepOriginId;
replorigin_session_origin_lsn = InvalidXLogRecPtr;
replorigin_session_origin_timestamp = 0;

replorigin_drop_by_name(originname, true, false);

/*
* Cleanup the tablesync slot.
*
* It is important to give an error if we are unable to drop the slot,
* otherwise, it won't be dropped till the corresponding subscription
* is dropped. So passing missing_ok = false.
*/
ReplicationSlotNameForTablesync(MyLogicalRepWorker->subid,
MyLogicalRepWorker->relid,
syncslotname,
sizeof(syncslotname));
ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);


------
Kind Regards,
Peter Smith.
Fujitsu Australia