Обсуждение: [PROPOSAL] comments in repl_scanner
I attached a simple patch extending the `replication/repl_scanner.l` with following test:
psql "dbname=postgres replication=database"
Border style is 2.
Line style is unicode.
psql (14.19 (Homebrew), server 19devel)
WARNING: psql major version 14, server major version 19.
Some psql features might not work.
Type "help" for help.
postgres=# -- foo
/* bar
/* blah
*/
*/
IDENTIFY_SYSTEM;
┌─────────────────────┬──────────┬────────────┬──────────┐
│ systemid │ timeline │ xlogpos │ dbname │
├─────────────────────┼──────────┼────────────┼──────────┤
│ 7561021876571120357 │ 1 │ 0/0176B5A8 │ postgres │
└─────────────────────┴──────────┴────────────┴──────────┘
(1 row)
```
Вложения
On Tue, Oct 14, 2025 at 01:13:38PM +0200, Matěj Klonfar wrote: > I can imagine this limitation is likely a holdover from the system's > evolution from physical replication where comments make no sense. However, > in logical replication walsender mode both SQL and replication statements > can be issued [1], so the current state brings the necessity to distinguish > when to inject the comment and when not to. What do you feel, are there any > unexpected impacts of extending the replication grammar with comments? > > I attached a simple patch extending the `replication/repl_scanner.l` with > following test: > > What do you feel, is that a good idea and/or are there any downsides I am > missing? Thank you all for the feedback. A downside here is the extra maintenance that this creates. I cannot get much excited about the support of comments in the context of replication commands, TBH. That's just one opinion, of course, others may have a different view. Note that even if one uses "replication=database", the code falls back to the main query parser, where comments work. FWIW, this is a bit like a past proposal with making the replication commands case-insensitive, back in 2017: https://www.postgresql.org/message-id/CAB7nPqSg2ZECE+ctGXieiCpVFibLyWgwAGaoEP3-zwYqJwaP-g@mail.gmail.com The argument for rejection back then is that these commands are not for manual consumption, so we should keep the replication grammar file simpler. Perhaps there could be an argument with allowing commands when it comes to the logging of replication commands in the server logs, where comments can be used as a reference. That's a very narrow case, so I'd still argue that this is not enough to balance in favor of this proposal. Just my 2c. -- Michael
Вложения
On Tue, Oct 14, 2025 at 01:13:38PM +0200, Matěj Klonfar wrote:
> I can imagine this limitation is likely a holdover from the system's
> evolution from physical replication where comments make no sense. However,
> in logical replication walsender mode both SQL and replication statements
> can be issued [1], so the current state brings the necessity to distinguish
> when to inject the comment and when not to. What do you feel, are there any
> unexpected impacts of extending the replication grammar with comments?
>
> I attached a simple patch extending the `replication/repl_scanner.l` with
> following test:
>
> What do you feel, is that a good idea and/or are there any downsides I am
> missing? Thank you all for the feedback.
A downside here is the extra maintenance that this creates. I cannot
get much excited about the support of comments in the context of
replication commands, TBH.
That's just one opinion, of course, others
may have a different view. Note that even if one uses
"replication=database", the code falls back to the main query parser,
where comments work.
psql "dbname=postgres replication=database"
Border style is 2.
Line style is unicode.
psql (14.19 (Homebrew), server 19devel)
WARNING: psql major version 14, server major version 19.
Some psql features might not work.
Type "help" for help.
postgres=# /* foo */ IDENTIFY_SYSTEM;
2025-10-16 09:49:59.152 CEST [49754] ERROR: syntax error at or near "IDENTIFY_SYSTEM" at character 11
2025-10-16 09:49:59.152 CEST [49754] STATEMENT: /* foo */ IDENTIFY_SYSTEM;
ERROR: syntax error at or near "IDENTIFY_SYSTEM"
LINE 1: /* foo */ IDENTIFY_SYSTEM;
```
FWIW, this is a bit like a past proposal with making the replication
commands case-insensitive, back in 2017:
https://www.postgresql.org/message-id/CAB7nPqSg2ZECE+ctGXieiCpVFibLyWgwAGaoEP3-zwYqJwaP-g@mail.gmail.com
The argument for rejection back then is that these commands are not
for manual consumption, so we should keep the replication grammar file
simpler. Perhaps there could be an argument with allowing commands
when it comes to the logging of replication commands in the server
logs, where comments can be used as a reference. That's a very narrow
case, so I'd still argue that this is not enough to balance in favor
of this proposal. Just my 2c.
--
Michael
On 14.10.25 13:13, Matěj Klonfar wrote: > certain instrumentation tools do prefix each statement with an > informational comment, typically to provide some tracing information to > logs (datadog for example). While this works for SQL statements, it's > not possible with logical replication statements because their grammar > doesn't support comments and it is causing unnecessary syntax errors. > > I can imagine this limitation is likely a holdover from the system's > evolution from physical replication where comments make no sense. > However, in logical replication walsender mode both SQL and replication > statements can be issued [1], so the current state brings the necessity > to distinguish when to inject the comment and when not to. What do you > feel, are there any unexpected impacts of extending the replication > grammar with comments? Another approach could be to get rid of repl_scanner.l and use the main scanner. This would be similar to how plpgsql works.