On Monday, January 30, 2023 12:13 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are my review comments for v88-0002.
Thanks for your comments.
>
> ======
> General
>
> 1.
> The test cases are checking the log content but they are not checking for
> debug logs or untranslated elogs -- they are expecting a normal ereport LOG
> that might be translated. I’m not sure if that is OK, or if it is a potential
> problem.
We have tests that check the ereport ERROR and ereport WARNING message(by
search for the ERROR or WARNING keyword for all the tap tests), so I think
checking the LOG should be fine.
> ======
> doc/src/sgml/config.sgml
>
> 2.
> On the publisher side, logical_replication_mode allows allows streaming or
> serializing changes immediately in logical decoding. 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, the decoding will stream or serialize changes when
> logical_decoding_work_mem is reached.
>
> 2a.
> typo "allows allows" (Kuroda-san reported same)
>
> 2b.
> "if streaming option" --> "if the streaming option"
Changed.
> ~~~
>
> 3.
> On the subscriber side, if streaming option is set to parallel,
> logical_replication_mode also allows the leader apply worker to send changes
> to the shared memory queue or to serialize changes.
>
> SUGGESTION
> On the subscriber side, if the streaming option is set to parallel,
> logical_replication_mode can be used to direct the leader apply worker to
> send changes to the shared memory queue or to serialize changes.
Changed.
> ======
> src/backend/utils/misc/guc_tables.c
>
> 4.
> {
> {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> - gettext_noop("Controls when to replicate each change."),
> - gettext_noop("On the publisher, it allows streaming or serializing each
> change in logical decoding."),
> + gettext_noop("Controls the internal behavior of logical replication
> publisher and subscriber"),
> + gettext_noop("On the publisher, it allows streaming or "
> + "serializing each change in logical decoding. On the "
> + "subscriber, in parallel streaming mode, it allows "
> + "the leader apply worker to serialize changes to "
> + "files and notifies the parallel apply workers to "
> + "read and apply them at the end of the transaction."),
> GUC_NOT_IN_SAMPLE
> },
> Suggest re-wording the long description (subscriber part) to be more like the
> documentation text.
>
> BEFORE
> On the subscriber, in parallel streaming mode, it allows the leader apply
> worker
> to serialize changes to files and notifies the parallel apply workers to read
> and
> apply them at the end of the transaction.
>
> SUGGESTION
> On the subscriber, if the streaming option is set to parallel, it directs the
> leader
> apply worker to send changes to the shared memory queue or to serialize
> changes and apply them at the end of the transaction.
>
Changed.
Attach the new version patch which addressed all comments so far (the v88-0001
has been committed, so we only have one remaining patch this time).
Best Regards,
Hou zj
v89-0001-Extend-the-logical_replication_mode-to-test-the-.patch
Description: v89-0001-Extend-the-logical_replication_mode-to-test-the-.patch
