On Thur, Feb 7, 2023 15:29 PM I wrote: > On Wed, Feb 1, 2023 20:07 PM Melih Mutlu <m.melihmu...@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