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

On Mon, Jan 23, 2023 21:00 PM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> wrote:
> Hi,
>
> Thanks for your review.
> Attached updated versions of the patches.

Thanks for updating the patch set.

> > 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.
>
> This "created_slot" indicates whether the current worker has created a
> replication slot for its own use. If so, created_slot will be true, otherwise false.
> Let's say the tablesync worker has not created its own slot yet in its previous
> runs or this is its first run. And the worker decides to reuse an existing
> replication slot (which created by another tablesync worker). Then created_slot
> is expected to be false. Because this particular tablesync worker has not created
> its own slot yet in either of its runs.
>
> In your example, the worker is in its first run and begin to sync a table whose
> state is FINISHEDCOPY. If the table's state is FINISHEDCOPY then the table
> should already have a replication slot created for its own sync process. The
> worker will want to reuse that existing replication slot for this particular table
> and it will not create a new replication slot. So created_slot will be false, because
> the worker has not actually created any replication slot yet.
>
> Basically, created_slot is set to true only if "walrcv_create_slot" is called by the
> tablesync worker any time during its lifetime. Otherwise, it's possible that the
> worker can use existing replication slots for each table it syncs. (e.g. if all the
> tables that the worker has synced were in FINISHEDCOPY state, then the
> worker will not need to create a new slot).
>
> Does it make sense now? Maybe I need to improve comments to make it
> clearer.

Yes, I think it makes sense. Thanks for the detailed explanation.
I think I misunderstood the second half of the comment. I previously thought it
meant that it was also true when reusing an existing slot.

I found one typo in v9-0002, but it seems already mentioned by Shi in [1].#5
before. Maybe you can have a look at that email for this and some other
comments.

Regards,
Wang Wei