On Wed, Jan 25, 2023 at 3:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jan 25, 2023 at 10:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Jan 25, 2023 at 3:15 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > 1.
> > > @@ -210,7 +210,7 @@ int logical_decoding_work_mem;
> > > static const Size max_changes_in_memory = 4096; /* XXX for restore only */
> > >
> > > /* GUC variable */
> > > -int logical_decoding_mode = LOGICAL_DECODING_MODE_BUFFERED;
> > > +int logical_replication_mode = LOGICAL_REP_MODE_BUFFERED;
> > >
> > >
> > > I noticed that the comment /* GUC variable */ is currently only above
> > > the logical_replication_mode, but actually logical_decoding_work_mem
> > > is a GUC variable too. Maybe this should be rearranged somehow then
> > > change the comment "GUC variable" -> "GUC variables"?
> > >
> >
> > I think moving these variables together doesn't sound like a good idea
> > because logical_decoding_work_mem variable is defined with other
> > related variable. Also, if we are doing the last comment, I think that
> > will obviate the need for this.
> >
> > > ======
> > > src/backend/utils/misc/guc_tables.c
> > >
> > > @@ -4908,13 +4908,13 @@ struct config_enum ConfigureNamesEnum[] =
> > > },
> > >
> > > {
> > > - {"logical_decoding_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> > > + {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> > > gettext_noop("Allows streaming or serializing each change in logical
> > > decoding."),
> > > NULL,
> > > GUC_NOT_IN_SAMPLE
> > > },
> > > - &logical_decoding_mode,
> > > - LOGICAL_DECODING_MODE_BUFFERED, logical_decoding_mode_options,
> > > + &logical_replication_mode,
> > > + LOGICAL_REP_MODE_BUFFERED, logical_replication_mode_options,
> > > NULL, NULL, NULL
> > > },
> > >
> > > That gettext_noop string seems incorrect. I think Kuroda-san
> > > previously reported the same, but then you replied it has been fixed
> > > already [1]
> > >
> > > > I felt the description seems not to be suitable for current behavior.
> > > > A short description should be like "Sets a behavior of logical replication", and
> > > > further descriptions can be added in lond description.
> > > I adjusted the description here.
> > >
> > > But this doesn't look fixed to me. (??)
> > >
> >
> > Okay, so, how about the following for the 0001 patch:
> > short desc: Controls when to replicate each change.
> > long desc: On the publisher, it allows streaming or serializing each
> > change in logical decoding.
> >
>
> I have updated the patch accordingly and it looks good to me. I'll
> push this first patch early next week (Monday) unless there are more
> comments.
The patch looks good to me too. Thank you for the patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com