On Thu, Dec 22, 2022 at 1:55 PM Kyotaro Horiguchi <horikyota....@gmail.com> wrote: > > At Thu, 22 Dec 2022 16:59:30 +0900, Masahiko Sawada <sawada.m...@gmail.com> > wrote in > > On Thu, Dec 22, 2022 at 4:18 PM Hayato Kuroda (Fujitsu) > > <kuroda.hay...@fujitsu.com> wrote: > > > > > > Dear Amit, > > > > > > Thank you for updating the patch. I have also checked the patch > > > and basically it has worked well. Almost all things I found were modified > > > by v4. > > > > > > One comment: while setting logical_decoding_mode to wrong value, > > > I got unfriendly ERROR message. > > > > > > ``` > > > postgres=# SET logical_decoding_mode = 1; > > > ERROR: invalid value for parameter "logical_decoding_mode": "1" > > > HINT: Available values: , immediate > > > ``` > > > > > > Here all acceptable enum should be output as HINT, but we could not see > > > the empty string. > > > Should we modify config_enum_get_options() for treating empty string, > > > maybe > > > like (empty)? > > > > Good point. I think the hint message can say "The only allowed value > > is \"immediate\" as recovery_target does. Or considering the name of > > logical_decoding_mode, we might want to have a non-empty string, say > > 'normal' as Kuroda-san proposed, as the default value. > > Oh. I missed this, and agree to have the explicit default option. > I slightly prefer "buffered" but "normal" also works fine for me. >
+1 for "buffered" as that seems to convey the meaning better. -- With Regards, Amit Kapila.
