On Tuesday, January 24, 2023 11:43 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for patch v86-0001.
Thanks for your comments.
>
>
> ======
> Commit message
>
> 2.
> Since we may extend the developer option logical_decoding_mode to to test the
> parallel apply of large transaction on subscriber, rename this option to
> logical_replication_mode to make it easier to understand.
>
> ~
>
> 2a
> typo "to to"
>
> typo "large transaction on subscriber" --> "large transactions on the subscriber"
>
> ~
>
> 2b.
> IMO better to rephrase the whole paragraph like shown below.
>
> SUGGESTION
>
> Rename the developer option 'logical_decoding_mode' to the more flexible
> name 'logical_replication_mode' because doing so will make it easier to extend
> this option in future to help test other areas of logical replication.
Changed.
> ======
> doc/src/sgml/config.sgml
>
> 3.
> Allows streaming or serializing changes immediately in logical decoding. The
> allowed values of logical_replication_mode are buffered and immediate. When
> set to immediate, stream each change if streaming option (see optional
> parameters set by CREATE SUBSCRIPTION) is enabled, otherwise, serialize each
> change. When set to buffered, which is the default, decoding will stream or
> serialize changes when logical_decoding_work_mem is reached.
>
> ~
>
> IMO it's more clear to say the default when the options are first mentioned. So I
> suggested removing the "which is the default" part, and instead saying:
>
> BEFORE
> The allowed values of logical_replication_mode are buffered and immediate.
>
> AFTER
> The allowed values of logical_replication_mode are buffered and immediate. The
> default is buffered.
I included this change in the 0002 patch.
> ======
> src/backend/utils/misc/guc_tables.c
>
> 4.
> @@ -396,8 +396,8 @@ static const struct config_enum_entry
> ssl_protocol_versions_info[] = { };
>
> static const struct config_enum_entry logical_decoding_mode_options[] = {
> - {"buffered", LOGICAL_DECODING_MODE_BUFFERED, false},
> - {"immediate", LOGICAL_DECODING_MODE_IMMEDIATE, false},
> + {"buffered", LOGICAL_REP_MODE_BUFFERED, false}, {"immediate",
> + LOGICAL_REP_MODE_IMMEDIATE, false},
> {NULL, 0, false}
> };
>
> I noticed this array is still called "logical_decoding_mode_options".
> Was that deliberate?
No, I didn't notice this one. Changed.
Best Regards,
Hou zj