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

On Wed, Jan 11, 2023 4:31 PM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> wrote:
> Rebased the patch to resolve conflicts.

Thanks for your patch set.

Here are some comments:

v3-0001* patch
===============

1. typedefs.list
I think we also need to add "walrcv_slot_snapshot_fn" to this file.

v7-0002* patch
===============
1. About function ReplicationOriginNameForLogicalRep()
Do we need to modify the API of this function? I think the original API could
also meet the current needs. Since this is not a static function, I think it
seems better to keep the original API if there is no reason. Please let me know
if I'm missing something.

-----

2. Comment atop the function GetSubscriptionRelReplicationSlot
+/*
+ * Get replication slot name of subscription table.
+ *
+ * Returns null if the subscription table does not have a replication slot.
+ */

Since this function always returns NULL, I think it would be better to say the
value in "slotname" here instead of the function's return value.

If you agree with this, please also kindly modify the comment atop the function
GetSubscriptionRelOrigin.

-----

3. typo
+ * At this point, there shouldn't be any existing replication
+ * origin wit the same name.

wit -> with

-----

4. In function CreateSubscription
+ values[Anum_pg_subscription_sublastusedid - 1] = Int64GetDatum(1);

I think it might be better to initialize this field to NULL or 0 here.
Because in the patch, we always ignore the initialized value when launching
the sync worker in the function process_syncing_tables_for_apply. And I think
we could document in pg-doc that this value means that no tables have been
synced yet.

-----

5. New member "created_slot" in structure LogicalRepWorker
+ /*
+ * Indicates if the sync worker created a replication slot or it reuses an
+ * existing one created by another worker.
+ */
+ bool created_slot;

I think the second half of the sentence looks inaccurate.
Because I think this flag could be false even when we reuse an existing slot
created by another worker. Assuming the first run for the worker tries to sync
a table which is synced by another sync worker before, and the relstate is set
to SUBREL_STATE_FINISHEDCOPY by another sync worker, I think this flag will not
be set to true. (see function LogicalRepSyncTableStart)

So, what if we simplify the description here and just say that this worker
already has it's default slot?

If I'm not missing something and you agree with this, please also kindly modify
the relevant comment atop the if-statement (!MyLogicalRepWorker->created_slot)
in the function LogicalRepSyncTableStart.

Regards,
Wang Wei