RE: [PATCH] Reuse Workers and Replication Slots during Logical Replication - Mailing list pgsql-hackers

On Thur, Feb 7, 2023 15:29 PM I wrote:
> On Wed, Feb 1, 2023 20:07 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> > Thanks for pointing out this review. I somehow skipped that, sorry.
> >
> > Please see attached patches.
> 
> Thanks for updating the patch set.
> Here are some comments.

Hi, here are some more comments on patch v7-0001*:

1. The new comments atop the function CreateDecodingContext
+ * need_full_snapshot
+ *         if true, create a snapshot able to read all tables,
+ *         otherwise do not create any snapshot.

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)

===

2. This are two questions I'm not sure about.
2a.
Because pg-doc has the following description in [1]: (option "SNAPSHOT 'use'")
```
'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")));
```

2b.
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.

Is there any reason not to do these two checks? Please let me know if I missed
something.

===

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.

[1] - https://www.postgresql.org/docs/devel/protocol-replication.html

Regards,
Wang Wei