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

Here are some review comments for the v2-0001 patch:

======

1. Commit message

The commit message should give some reason why relocating the origin
slot drop code is expected to fix the reported problem.

======

2. src/backend/replication/logical/tablesync.c

+ /* reset the origin session before dropping */
+ replorigin_session_reset();
+
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;

2a.
Uppercase comment

2b.
IIUC you are not doing the reset because you particularly want to do a
reset, but really because this is the only (?) way to dis-associate
the current process from owning the slot. Otherwise, the slot would be
considered still "busy" and the subsequent replorigin_drop_by_name
would be rejected. I thought the current comment should be expanded to
explain all this background.

2c.
Perhaps it is non-standard to do so, but I wondered if you can just
call pg_replication_origin_session_reset instead of
replorigin_session_reset here so that there would only be 1 LOC
instead of 4.

~~~

3. src/backend/replication/logical/tablesync.c

+ /*
* Cleanup the tablesync slot.
*
* This has to be done after updating the state because otherwise if

Doesn't the same note ("This has to be done after...") also apply to
the code dropping the origin slot? Maybe this note needs to be either
duplicated or just put a common note about this above both of those
slot drops.

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