Hello Robert,
My 0.02 €:
It seems to me that we're making the same mistake with the replication
parser that we've made in various placesin the regular parser: using a
syntax for options that requires that every potential option be a
keyword, and every potential option requires modification of the
grammar. In particular, the syntax for the BASE_BACKUP command has
accreted a whole lot of cruft already and I think that trend is likely
to continue. I don't think that trying to keep people from adding
options is a good strategy,
Indeed.
so instead I'd like to have a better way to do it.
Attached is v1 of a patch to refactor things so that parts of the
BASE_BACKUP and CREATE_REPLICATION_SLOT are replaced with a flexible
options syntax.
Patch applies cleanly, however compilation fails on:
repl_gram.y:271:106: error: expected ‘;’ before ‘}’
Getting rid of "ident_or_keyword", some day, would be a relief.
For boolean options, you are generating (EXPORT_SNAPSHOT TRUE) where
(EXPORT_SNAPSHOT) would do.
Maybe allowing (!EXPORT_SNAPSHOT) for (FOO FALSE) would be nice, if it is
not allowed yet. That would also allow to get rid of FOO/NOFOO variants if
any for FOO & !FOO, so one keyword is enough for a concept.
There are some debatable decisions here, so I'd be happy to get some
feedback on whether to go further with this, or less far, or maybe even
just abandon the idea altogether. I doubt the last one is the right
course, though: ISTM something's got to be done about the BASE_BACKUP
case, at least.
ISTM that it would be better to generalize the approach to all commands
which accept options, so that the syntax is homogeneous.
If people agree with the basic approach, I'll write docs. The
intention is that we'd continue to support the existing syntax for the
existing options, but the client tools would be adjusted to use the
new syntax if the server's new enough, and any new options would be
supported only through the new syntax.
Yes.
At some point in the distant future we could retire the old syntax, when
we've stopped caring about compatibility with pre-14 versions.
Just wondering: ISTM that the patch implies that dumping a v14 db
generates the new syntax, which makes sense. Now I see 4 use cases wrt to
version.
# source target comment
1 v < 14 v < 14 probably the dump would use one of the older version
2 v < 14 v >= 14 upgrade
3 v >= 14 v < 14 downgrade: oops, the output uses the new syntax
4 v >= 14 v >= 14 ok
Both cross version usages may be legitimate. In particular, 3 (oops,
hardware issue, I have to move the db to a server where pg has not been
upgraded) seems not possible because the generated syntax uses the new
approach. Should/could there be some option to tell "please generate vXXX
syntax" to allow that?
--
Fabien.
