On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli <e...@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.
~~~
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
------
test_pub=# SELECT * FROM pg_logical_slot_get_changes('test_slot_v1',
NULL, NULL, 'banana', '1');
2023-12-19 17:08:21.627 AEDT [8921] ERROR: unrecognized pgoutput option: banana
2023-12-19 17:08:21.627 AEDT [8921] CONTEXT: slot "test_slot_v1",
output plugin "pgoutput", in the startup callback
2023-12-19 17:08:21.627 AEDT [8921] STATEMENT: SELECT * FROM
pg_logical_slot_get_changes('test_slot_v1', NULL, NULL, 'banana',
'1');
ERROR: unrecognized pgoutput option: banana
CONTEXT: slot "test_slot_v1", output plugin "pgoutput", in the startup callback
------
But that doesn't seem right. AFAIK elog messages use errmsg_internal
so this message would not get translated.
PSA a patch to fix that.
======
[1] code coverage --
https://coverage.postgresql.org/src/backend/replication/pgoutput/pgoutput.c.gcov.html
Kind Regards,
Peter Smith.
Fujitsu Australia
parse_output_parameters.diff
Description: Binary data
