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.
--
With Regards,
Amit Kapila.