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

Hi Wang,

Thanks for reviewing.
Please see updated patches. [1]

wangw(dot)fnst(at)fujitsu(dot)com <wangw(dot)fnst(at)fujitsu(dot)com>, 7 Şub 2023 Sal, 10:28
tarihinde şunu yazdı:

> 1. In the function ApplyWorkerMain.
> I think we need to keep the worker name as "leader apply worker" in the
> comment
> like the current HEAD.
>

Done.

> I think in this case we also need to pop the error context stack before
> returning. Otherwise, I think we might use the wrong callback
> (apply error_callback) after we return from this function.
>

Done.

> 3. About the function UpdateSubscriptionRelReplicationSlot.
> This newly introduced function UpdateSubscriptionRelReplicationSlot does
> not
> seem to be invoked. Do we need this function?

Removed.

I think if 'need_full_snapshot' is false, it means we will create a snapshot
> that can read only catalogs. (see SnapBuild->building_full_snapshot)

Fixed.

```
> 'use' will use the snapshot for the current transaction executing the
> command.
> This option must be used in a transaction, and CREATE_REPLICATION_SLOT
> must be
> the first command run in that transaction.
> ```

So I think in the function CreateDecodingContext, when "need_full_snapshot"
> is
> true, we seem to need the following check, just like in the function
> CreateInitDecodingContext:

```
> if (IsTransactionState() &&
> GetTopTransactionIdIfAny() != InvalidTransactionId)
> ereport(ERROR,
> (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
> errmsg("cannot create logical replication
> slot in transaction that has performed writes")));
> ```

You're right to "use" the snapshot, it must be the first command in the
transaction. And that check happens here [2]. CreateReplicationSnapshot has
also similar check.
I think the check you're referring to is needed to actually create a
replication slot and it performs whether the snapshot will be "used" or
"exported". This is not the case for CreateReplicationSnapshot.

It seems that we also need to invoke the function
> CheckLogicalDecodingRequirements in the new function
> CreateReplicationSnapshot,
> just like the function CreateReplicationSlot and the function
> StartLogicalReplication.

Added this check.

3. The invocation of startup_cb_wrapper in the function
> CreateDecodingContext.
> I think we should change the third input parameter to true when invoke
> function
> startup_cb_wrapper for CREATE_REPLICATION_SNAPSHOT. BTW, after applying
> patch
> v10-0002*, these settings will be inconsistent when sync workers use
> "CREATE_REPLICATION_SLOT" and "CREATE_REPLICATION_SNAPSHOT" to take
> snapshots.
> This input parameter (true) will let us disable streaming and two-phase
> transactions in function pgoutput_startup. See the last paragraph of the
> commit
> message for 4648243 for more details.

I'm not sure if "is_init" should be set to true. CreateDecodingContext only
creates a context for an already existing logical slot and does not
initialize new one.
I think inconsistencies between "CREATE_REPLICATION_SLOT" and
"CREATE_REPLICATION_SNAPSHOT" are expected since one creates a new
replication slot and the other does not.
CreateDecodingContext is also used in other places as well. Not sure how
this change would affect those places. I'll look into this more. Please let
me know if I'm missing something.

[1]
https://www.postgresql.org/message-id/CAGPVpCQmEE8BygXr%3DHi2N2t2kOE%3DPJwofn9TX0J9J4crjoXarQ%40mail.gmail.com
[2]
https://github.com/postgres/postgres/blob/master/src/backend/replication/walsender.c#L1108

Thanks,
--
Melih Mutlu
Microsoft