Hi Michael, Thanks for your notes.
čt 16. 10. 2025 v 6:08 odesílatel Michael Paquier <[email protected]> napsal: > 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. Makes sense, on the other hand, the most of the work was done in e4a8fb8fefb9 where `yyextra` was introduced to handle the context information (necessary for nested `/* */` comments). This proposal is then, in my opinion, just a small enhancement built on top of that with no significant impact to the maintenance costs. > 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. > I do not agree. Tried now on the current master HEAD (commit 41c674d2): ``` 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; ``` Matej > 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-zwyqjwa...@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 >
