Обсуждение: logical_replication_mode
I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with. I
think this is similar to force_parallel_mode.
Also, the descriptions in guc_tables.c could be improved. For example,
gettext_noop("Controls when to replicate or apply each change."),
is pretty content-free and unhelpful.
On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I suggest we rename this setting to something starting with debug_.
> Right now, the name looks much too tempting for users to fiddle with. I
> think this is similar to force_parallel_mode.
>
+1. How about debug_logical_replication?
> Also, the descriptions in guc_tables.c could be improved. For example,
>
> gettext_noop("Controls when to replicate or apply each change."),
>
> is pretty content-free and unhelpful.
>
The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you have
any better ideas?
--
With Regards,
Amit Kapila.
On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
> wrote:
> >
> > I suggest we rename this setting to something starting with debug_.
> > Right now, the name looks much too tempting for users to fiddle with.
> > I think this is similar to force_parallel_mode.
> >
>
> +1. How about debug_logical_replication?
>
> > Also, the descriptions in guc_tables.c could be improved. For
> > example,
> >
> > gettext_noop("Controls when to replicate or apply each change."),
> >
> > is pretty content-free and unhelpful.
> >
>
> The other possibility I could think of is to change short_desc as:
> "Allows to replicate each change for large transactions.". Do you have any
> better ideas?
How about "Forces immediate streaming or serialization of changes in large
transactions." which is similar to the description in document.
I agree that renaming it to debug_xx would be better and
here is a patch that tries to do this.
Best Regards,
Hou zj
Вложения
On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
> On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
>> wrote:
>>>
>>> I suggest we rename this setting to something starting with debug_.
>>> Right now, the name looks much too tempting for users to fiddle with.
>>> I think this is similar to force_parallel_mode.
>>>
>>
>> +1. How about debug_logical_replication?
>>
>>> Also, the descriptions in guc_tables.c could be improved. For
>>> example,
>>>
>>> gettext_noop("Controls when to replicate or apply each change."),
>>>
>>> is pretty content-free and unhelpful.
>>>
>>
>> The other possibility I could think of is to change short_desc as:
>> "Allows to replicate each change for large transactions.". Do you have any
>> better ideas?
>
> How about "Forces immediate streaming or serialization of changes in large
> transactions." which is similar to the description in document.
>
> I agree that renaming it to debug_xx would be better and
> here is a patch that tries to do this.
Maybe debug_logical_replication is too general? Something like
debug_logical_replication_streaming would be more concrete. (Or
debug_logical_streaming.) Is that an appropriate name for what it's doing?
On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
> > On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
> >> wrote:
> >>>
> >>> I suggest we rename this setting to something starting with debug_.
> >>> Right now, the name looks much too tempting for users to fiddle with.
> >>> I think this is similar to force_parallel_mode.
> >>>
> >>
> >> +1. How about debug_logical_replication?
> >>
> >>> Also, the descriptions in guc_tables.c could be improved. For
> >>> example,
> >>>
> >>> gettext_noop("Controls when to replicate or apply each change."),
> >>>
> >>> is pretty content-free and unhelpful.
> >>>
> >>
> >> The other possibility I could think of is to change short_desc as:
> >> "Allows to replicate each change for large transactions.". Do you have any
> >> better ideas?
> >
> > How about "Forces immediate streaming or serialization of changes in large
> > transactions." which is similar to the description in document.
> >
> > I agree that renaming it to debug_xx would be better and
> > here is a patch that tries to do this.
>
> Maybe debug_logical_replication is too general? Something like
> debug_logical_replication_streaming would be more concrete.
>
Yeah, that sounds better.
> (Or
> debug_logical_streaming.) Is that an appropriate name for what it's doing?
>
Yes.
--
With Regards,
Amit Kapila.
On Friday, August 25, 2023 5:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
> > > On Friday, August 25, 2023 12:28 PM Amit Kapila
> <amit.kapila16@gmail.com> wrote:
> > >>
> > >> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut
> > >> <peter@eisentraut.org>
> > >> wrote:
> > >>>
> > >>> I suggest we rename this setting to something starting with debug_.
> > >>> Right now, the name looks much too tempting for users to fiddle with.
> > >>> I think this is similar to force_parallel_mode.
> > >>>
> > >>
> > >> +1. How about debug_logical_replication?
> > >>
> > >>> Also, the descriptions in guc_tables.c could be improved. For
> > >>> example,
> > >>>
> > >>> gettext_noop("Controls when to replicate or apply each
> > >>> change."),
> > >>>
> > >>> is pretty content-free and unhelpful.
> > >>>
> > >>
> > >> The other possibility I could think of is to change short_desc as:
> > >> "Allows to replicate each change for large transactions.". Do you
> > >> have any better ideas?
> > >
> > > How about "Forces immediate streaming or serialization of changes in
> > > large transactions." which is similar to the description in document.
> > >
> > > I agree that renaming it to debug_xx would be better and here is a
> > > patch that tries to do this.
> >
> > Maybe debug_logical_replication is too general? Something like
> > debug_logical_replication_streaming would be more concrete.
> >
>
> Yeah, that sounds better.
OK, here is the debug_logical_replication_streaming version.
Best Regards,
Hou zj
Вложения
Hi Hou-san.
I had a look at the patch 0001.
It looks OK to me, but here are a couple of comments:
======
1. Is this fix intended for PG16?
I found some mention of this GUC old name lurking in the release v16 notes [1].
~~~
2. DebugLogicalRepStreamingMode
-/* possible values for logical_replication_mode */
+/* possible values for debug_logical_replication_streaming */
typedef enum
{
- LOGICAL_REP_MODE_BUFFERED,
- LOGICAL_REP_MODE_IMMEDIATE
-} LogicalRepMode;
+ DEBUG_LOGICAL_REP_STREAMING_BUFFERED,
+ DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE
+} DebugLogicalRepStreamingMode;
Shouldn't this typedef name be included in the typedef.list file?
------
[1] https://www.postgresql.org/docs/16/release-16.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Aug 29, 2023 at 12:56 PM Peter Smith <smithpb2250@gmail.com> wrote: > > I had a look at the patch 0001. > > It looks OK to me, but here are a couple of comments: > > ====== > > 1. Is this fix intended for PG16? > Yes. > I found some mention of this GUC old name lurking in the release v16 notes [1]. > That should be changed as well but we can do that as a separate patch just for v16. -- With Regards, Amit Kapila.
On Tuesday, August 29, 2023 3:26 PM Peter Smith <smithpb2250@gmail.com> wrote:
Thanks for reviewing.
> 2. DebugLogicalRepStreamingMode
>
> -/* possible values for logical_replication_mode */
> +/* possible values for debug_logical_replication_streaming */
> typedef enum
> {
> - LOGICAL_REP_MODE_BUFFERED,
> - LOGICAL_REP_MODE_IMMEDIATE
> -} LogicalRepMode;
> + DEBUG_LOGICAL_REP_STREAMING_BUFFERED,
> + DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE
> +} DebugLogicalRepStreamingMode;
>
> Shouldn't this typedef name be included in the typedef.list file?
I think it's unnecessary to add this as there currently is no reference to the name.
See other similar examples like DebugParallelMode, RecoveryPrefetchValue ...
And the name is also not included in BF[1]
[1] https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?branch=HEAD
Best Regards,
Hou zj
On 27.08.23 14:05, Zhijie Hou (Fujitsu) wrote:
> On Friday, August 25, 2023 5:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>>
>>> On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
>>>> On Friday, August 25, 2023 12:28 PM Amit Kapila
>> <amit.kapila16@gmail.com> wrote:
>>>>>
>>>>> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut
>>>>> <peter@eisentraut.org>
>>>>> wrote:
>>>>>>
>>>>>> I suggest we rename this setting to something starting with debug_.
>>>>>> Right now, the name looks much too tempting for users to fiddle with.
>>>>>> I think this is similar to force_parallel_mode.
>>>>>>
>>>>>
>>>>> +1. How about debug_logical_replication?
>>>>>
>>>>>> Also, the descriptions in guc_tables.c could be improved. For
>>>>>> example,
>>>>>>
>>>>>> gettext_noop("Controls when to replicate or apply each
>>>>>> change."),
>>>>>>
>>>>>> is pretty content-free and unhelpful.
>>>>>>
>>>>>
>>>>> The other possibility I could think of is to change short_desc as:
>>>>> "Allows to replicate each change for large transactions.". Do you
>>>>> have any better ideas?
>>>>
>>>> How about "Forces immediate streaming or serialization of changes in
>>>> large transactions." which is similar to the description in document.
>>>>
>>>> I agree that renaming it to debug_xx would be better and here is a
>>>> patch that tries to do this.
>>>
>>> Maybe debug_logical_replication is too general? Something like
>>> debug_logical_replication_streaming would be more concrete.
>>>
>>
>> Yeah, that sounds better.
>
> OK, here is the debug_logical_replication_streaming version.
committed