On Tue, Dec 19, 2023 at 6:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 19, 2023 at 12:07 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli <emre@hasegeli.com> wrote:
> > >
> > > > Fair enough. I think we should push your first patch only in HEAD as
> > > > this is a minor improvement over the current behaviour. What do you
> > > > think?
> > >
> > > I agree.
> >
> > Patch 0001
> >
> > AFAICT parse_output_parameters possible errors are never tested. For
> > example, there is no code coverage [1] touching any of these ereports.
> >
> > IMO there should be some simple test cases -- I am happy to create
> > some tests if you agree they should exist.
> >
>
> I don't think having tests for all sorts of error checking will add
> much value as compared to the overhead they bring.
>
> > ~~~
> >
> > While looking at the function parse_output_parameters() I noticed that
> > if an unrecognised option is passed the function emits an elog instead
> > of an ereport
> >
>
> We don't expect unrecognized option here and for such a thing, we use
> elog in the code. See the similar usage in
> parseCreateReplSlotOptions().
>
IIUC the untranslated elog should be used for internal/sanity errors,
debugging, or stuff that cannot happen under any normal circumstances.
While that may be the case for parseCreateReplSlotOptions() mentioned,
IMO the scenario in the parse_output_parameters() is very different,
because these options can come directly from user input so any user
typo can cause this error. Indeed, this is probably one of the more
likely reasons for getting any error in parse_output_parameters()
function. I thought any errors that can be easily caused by some user
actions ought to be translated.
For example, the user accidentally misspells 'proto_version':
test_pub=# SELECT * FROM pg_logical_slot_get_changes('test_slot_v1',
NULL, NULL, 'protocol_version', '1');
ERROR: unrecognized pgoutput option: protocol_version
CONTEXT: slot "test_slot_v1", output plugin "pgoutput", in the startup callback
======
Kind Regards,
Peter Smith.
Fujitsu Australia