Re: extensible options syntax for replication parser?

On Sun, Jun 14, 2020 at 3:15 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> > 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 ‘}’

Oops. I'm surprised my compiler didn't complain.

> Getting rid of "ident_or_keyword", some day, would be a relief.

Actually, I think that particular thing is a sign that things are
being done correctly. You need something like that if you have
contexts where you want to treat keywords and non-keywords the same
way, and it's generally good to have such places. In fact, this could
probably be profitably used in more places in the replication grammar.

> For boolean options, you are generating (EXPORT_SNAPSHOT TRUE) where
> (EXPORT_SNAPSHOT) would do.

True, but it doesn't seem to matter much one way or the other. I
thought this way looked a little clearer.

> 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.

Well, the goal for this is not to need ANY new keywords for a new
concept, but !FOO would be inconsistent with other places where we do
similar things (e.g. EXPLAIN, VACUUM, COPY) so I don't think that's
the way to go.

> > 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.

As a general principle, sure, but it's always debatable how far to
take things in particular cases. For instance, in the cases of
EXPLAIN, VACUUM, and COPY, the relation name is given as a dedicated
piece of syntax, not an option. It could be given as an option, but
since it's mandatory and important, we didn't. In the case of COPY,
the source file is also specified via dedicated syntax, rather than an
option. So we have to make the same kinds of decisions here. For
example, for CREATE_REPLICATION_SLOT, one could argue that PHYSICAL
and LOGICAL should be moved to the extensible options list instead of
being kept as separate syntax. However, that seems somewhat
inconsistent with the long-standing syntax for START_REPLICATION,
which already does use extensible options:

START_REPLICATION SLOT slot_name LOGICAL XXX/XXX [ ( option_name
[option_value] [, ... ] ) ]

On balance I am comfortable with what the patch does, but other people
might have a different take.

> 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?

I don't think dumping a DB is really affected by any of this. AFAIK,
replication commands aren't used in pg_dump output. It just affects
pg_basebackup and the server, and you'll notice that I have taken
pains to allow the server to continue to accept the current format,
and to allow pg_basebackup to generate that format when talking to an
older server.

Thanks for the review. v2 attached, hopefully fixing the compilation
issue you mentioned.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company