extensible options syntax for replication parser?

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.

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

On Wed, Jun 24, 2020 at 11:51 AM Robert Haas <robertmhaas@gmail.com> wrote:

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

Tushar Ahuja reported to me off-list that my basebackup refactoring
patch set was changing whether or not the following message appeared:

NOTICE: WAL archiving is not enabled; you must ensure that all
required WAL segments are copied through other means to complete the
backup

That patch set includes this patch, and the reason for the behavior
difference turned out to be that I had gotten an if-test that is part
of this patch backwards. Here is v3, fixing that. It is a little
disappointing that this mistake didn't cause any existing regression
tests to fail.

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

On Thu, Oct 8, 2020 at 10:33 AM Robert Haas <robertmhaas@gmail.com> wrote:

That patch set includes this patch, and the reason for the behavior
difference turned out to be that I had gotten an if-test that is part
of this patch backwards. Here is v3, fixing that. It is a little
disappointing that this mistake didn't cause any existing regression
tests to fail.

I'm returning to this topic ~11 months later with a more definite
intent to get something committed, since my "refactoring basebackup.c"
patch set - that also adds server-side compression and server-side
backup - needs to add more options to BASE_BACKUP, and doubling down
on the present options-parsing strategy seems like a horrible idea.
I've now split this into two patches, one for BASE_BACKUP, and the
other for CREATE_REPLICATION_SLOT. I've rebased the patches and added
documentation as well. The CREATE_REPLICATION_SLOT patch now unifies
EXPORT_SNAPSHOT, NOEXPORT_SNAPSHOT, and USE_SNAPSHOT, which are
mutually exclusive choices, into SNAPSHOT { 'export' | 'use' |
'nothing' } which IMHO is clearer.

Last call for complaints about either the overall direction or the
specific implementation choices...

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Sep 10, 2021 at 3:44 PM Robert Haas <robertmhaas@gmail.com> wrote:

Last call for complaints about either the overall direction or the
specific implementation choices...

A complaint showed up over at
/messages/by-id/979131631633278@mail.yandex.ru and pursuant to that
complaint I have made the new syntax for controlling the checkpoint
type look like CHECKPOINT { 'fast' | 'spread' } rather than just
having an option called FAST. It was suggested over there to also
rename WAIT to WAIT_WAL_ARCHIVED, but I don't like that for reasons
explained on that thread and so have not adopted that proposal.

Sergei also helpfully pointed out that I'd accidentally deleted a
version check in one place, so this version is also updated to not do
that.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hello

Thanks, I missed this thread.

+ <term><literal>CHECKPOINT { 'fast' | 'spread' }</replaceable></literal></term>

Unpaired </replaceable> tag in docs.

That was all I noticed in 0001. Still not sure where is the difference between "change NOWAIT to WAIT" and "change NOWAIT to something else descriptive". But fine, I can live with WAIT. (one note: the exact command is visible to the user when log_replication_commands is enabled, not completely hidden)

0002 breaks "create subscription (with create_slot true)" when the publish server is an earlier version:

postgres=# create subscription test CONNECTION 'host=127.0.0.1 user=postgres' PUBLICATION test with (create_slot = true);
ERROR: could not create replication slot "test": ERROR: syntax error

regards, Sergei

On Wed, Sep 22, 2021 at 8:11 AM Sergei Kornilov <sk@zsrv.org> wrote:

+ <term><literal>CHECKPOINT { 'fast' | 'spread' }</replaceable></literal></term>

Unpaired </replaceable> tag in docs.

That was all I noticed in 0001. Still not sure where is the difference between "change NOWAIT to WAIT" and "change NOWAIT to something else descriptive". But fine, I can live with WAIT. (one note: the exact command is visible to the user when log_replication_commands is enabled, not completely hidden)

0002 breaks "create subscription (with create_slot true)" when the publish server is an earlier version:

postgres=# create subscription test CONNECTION 'host=127.0.0.1 user=postgres' PUBLICATION test with (create_slot = true);
ERROR: could not create replication slot "test": ERROR: syntax error

Thanks. I have attempted to fix these problems in the attached version.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 9/23/21 1:25 AM, Robert Haas wrote:

postgres=# create subscription test CONNECTION 'host=127.0.0.1 user=postgres' PUBLICATION test with (create_slot = true);
ERROR: could not create replication slot "test": ERROR: syntax error

Thanks. I have attempted to fix these problems in the attached version.

l checked and look like the issue is still not fixed against v7-* patches -

postgres=#  create subscription test CONNECTION 'host=127.0.0.1 user=edb
dbname=postgres' PUBLICATION p with (create_slot = true);
ERROR:  could not create replication slot "test": ERROR:  syntax error

--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Thu, Sep 23, 2021 at 2:55 AM tushar <tushar.ahuja@enterprisedb.com> wrote:

l checked and look like the issue is still not fixed against v7-* patches -

postgres=# create subscription test CONNECTION 'host=127.0.0.1 user=edb dbname=postgres' PUBLICATION p with (create_slot = true);
ERROR: could not create replication slot "test": ERROR: syntax error

Thanks. Looks like that version had some stupid mistakes. Here's a new one.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2021/09/24 0:05, Robert Haas wrote:

On Thu, Sep 23, 2021 at 2:55 AM tushar <tushar.ahuja@enterprisedb.com> wrote:

l checked and look like the issue is still not fixed against v7-* patches -

postgres=# create subscription test CONNECTION 'host=127.0.0.1 user=edb dbname=postgres' PUBLICATION p with (create_slot = true);
ERROR: could not create replication slot "test": ERROR: syntax error

Thanks. Looks like that version had some stupid mistakes. Here's a new one.

-     <indexterm><primary>BASE_BACKUP</primary></indexterm>
+    <term><literal>BASE_BACKUP</literal> [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ]

You seem to accidentally remove the index term for BASE_BACKUP.

+ident_or_keyword:
+ IDENT { $$ = $1; }

ident_or_keyword seems to be used only for generic options,
but it includes the keywords for legacy options like "FAST".
Isn't it better to remove the keywords for legacy options from
ident_or_keyword?

OTOH, the keywords for newly-introduced generic options like
CHECKPOINT should be defined in repl_scanner.l and repl_gram.y?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 9/23/21 8:35 PM, Robert Haas wrote:

Thanks. Looks like that version had some stupid mistakes. Here's a new one.

Thanks, the reported issue seems to be fixed now for HEAD w/patch
(publication) to HEAD w/patch (subscription) but still getting the same
error if we try to perform v12(publication) to HEAD
w/patch(subscription) . I checked there is no such issue for
v12(publication) to v14 RC1 (subscription)

postgres=#  create subscription sub123s CONNECTION 'host=127.0.0.1
user=edb  port=4444 dbname=postgres' PUBLICATION pp with (slot_name =
from_v14);
ERROR:  could not create replication slot "from_v14": ERROR: syntax error
postgres=#

--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Fri, Sep 24, 2021 at 12:01 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:

You seem to accidentally remove the index term for BASE_BACKUP.

Good catch.

+ident_or_keyword:
+                       IDENT                                                   { $$ = $1; }

ident_or_keyword seems to be used only for generic options,
but it includes the keywords for legacy options like "FAST".
Isn't it better to remove the keywords for legacy options from
ident_or_keyword?

I don't think so. I mean, if we do, then it's not really an
ident_or_keyword production any more, because it would only allow some
keywords, not all. Now if the keywords that are not included aren't
used as options anywhere then it won't matter, but it seems cleaner to
me to make the list complete.

OTOH, the keywords for newly-introduced generic options like
CHECKPOINT should be defined in repl_scanner.l and repl_gram.y?

One big advantage of this approach is that we don't need to make
changes to those files in order to add new options, so I feel like
that would be missing the point completely.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Sep 24, 2021 at 7:28 AM tushar <tushar.ahuja@enterprisedb.com> wrote:

On 9/23/21 8:35 PM, Robert Haas wrote:

Thanks. Looks like that version had some stupid mistakes. Here's a new one.

Thanks, the reported issue seems to be fixed now for HEAD w/patch
(publication) to HEAD w/patch (subscription) but still getting the same
error if we try to perform v12(publication) to HEAD
w/patch(subscription) . I checked there is no such issue for
v12(publication) to v14 RC1 (subscription)

postgres=# create subscription sub123s CONNECTION 'host=127.0.0.1
user=edb port=4444 dbname=postgres' PUBLICATION pp with (slot_name =
from_v14);
ERROR: could not create replication slot "from_v14": ERROR: syntax error
postgres=#

I am not able to reproduce this failure. I suspect you made a mistake
in testing, because my test case before sending the patch was
basically the same as yours, except that I was testing with v13. But I
tried again with v12 and it seems fine:

[rhaas pgsql]$ createdb -p 5412
[rhaas pgsql]$ psql -c 'select version()' -p 5412
version
----------------------------------------------------------------------------------------------------------------
PostgreSQL 12.3 on x86_64-apple-darwin19.4.0, compiled by clang
version 5.0.2 (tags/RELEASE_502/final), 64-bit
(1 row)
[rhaas pgsql]$ psql
psql (15devel)
Type "help" for help.

rhaas=# create subscription sub123s CONNECTION 'port=5412' PUBLICATION
pp with (slot_name =
from_v14);
NOTICE: created replication slot "from_v14" on publisher
CREATE SUBSCRIPTION

Here's v9, fixing the issue reported by Fujii Masao.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 9/24/21 10:36 PM, Robert Haas wrote:

I am not able to reproduce this failure. I suspect you made a mistake
in testing, because my test case before sending the patch was
basically the same as yours, except that I was testing with v13. But I
tried again with v12 and it seems fine:

Right, on a clean setup -I am not also not able to reproduce this issue.
Thanks for checking at your end.

--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On 9/24/21 10:36 PM, Robert Haas wrote:

Here's v9, fixing the issue reported by Fujii Masao.

Please refer this scenario where publication on v14RC1  and subscription
on HEAD (w/patch)

--create a subscription with parameter two_phase=1 on HEAD

postgres=# CREATE SUBSCRIPTION r1015 CONNECTION 'dbname=postgres
host=localhost port=5454' PUBLICATION p WITH (two_phase=1);
NOTICE:  created replication slot "r1015" on publisher
CREATE SUBSCRIPTION
postgres=#

--check on 14RC1

postgres=# select two_phase from pg_replication_slots where
slot_name='r105';
 two_phase
-----------
 f
(1 row)

so are we silently ignoring this parameter as it is not supported on
v14RC/HEAD ? and if yes then why not we just throw an message like
ERROR:  unrecognized subscription parameter: "two_phase"

--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On 9/24/21 11:57 PM, tushar wrote:

postgres=# select two_phase from pg_replication_slots where
slot_name='r105';

Correction -Please read  'r105' to 'r1015'

--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Fri, Sep 24, 2021 at 2:28 PM tushar <tushar.ahuja@enterprisedb.com> wrote:

Please refer this scenario where publication on v14RC1 and subscription
on HEAD (w/patch)

--create a subscription with parameter two_phase=1 on HEAD

postgres=# CREATE SUBSCRIPTION r1015 CONNECTION 'dbname=postgres
host=localhost port=5454' PUBLICATION p WITH (two_phase=1);
NOTICE: created replication slot "r1015" on publisher
CREATE SUBSCRIPTION
postgres=#

--check on 14RC1

postgres=# select two_phase from pg_replication_slots where
slot_name='r105';
two_phase
-----------
f
(1 row)

so are we silently ignoring this parameter as it is not supported on
v14RC/HEAD ? and if yes then why not we just throw an message like
ERROR: unrecognized subscription parameter: "two_phase"

two_phase is new in v15, something you could also find out by checking
the documentation. Now if the patch changes the way two_phase
interacts with older versions, that's a bug in the patch and we should
fix it. But if the same issue exists without the patch then I'm not
sure why you are raising it here rather than on the thread where that
feature was developed.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sat, Sep 25, 2021 at 4:28 AM tushar <tushar.ahuja@enterprisedb.com> wrote:

On 9/24/21 10:36 PM, Robert Haas wrote:

Here's v9, fixing the issue reported by Fujii Masao.

Please refer this scenario where publication on v14RC1 and subscription
on HEAD (w/patch)

--create a subscription with parameter two_phase=1 on HEAD

postgres=# CREATE SUBSCRIPTION r1015 CONNECTION 'dbname=postgres
host=localhost port=5454' PUBLICATION p WITH (two_phase=1);
NOTICE: created replication slot "r1015" on publisher
CREATE SUBSCRIPTION
postgres=#

--check on 14RC1

postgres=# select two_phase from pg_replication_slots where
slot_name='r105';
two_phase
-----------
f
(1 row)

so are we silently ignoring this parameter as it is not supported on
v14RC/HEAD ? and if yes then why not we just throw an message like
ERROR: unrecognized subscription parameter: "two_phase"

--

There is usually a time lag between a subscription created with two_phase on and
the slot on the publisher enabling two_phase. It only happens after a
tablesync is completed and
the apply worker is restarted. There are logs which indicate that this
has happened. If you could share the
logs (on publisher and subscriber) when this happens, I could have a look.

regards,
Ajin Cherian
Fujitsu Australia

On Mon, Sep 27, 2021 at 11:20 AM Ajin Cherian <itsajin@gmail.com> wrote:

On Sat, Sep 25, 2021 at 4:28 AM tushar <tushar.ahuja@enterprisedb.com> wrote:

On 9/24/21 10:36 PM, Robert Haas wrote:

Here's v9, fixing the issue reported by Fujii Masao.

Please refer this scenario where publication on v14RC1 and subscription
on HEAD (w/patch)

--create a subscription with parameter two_phase=1 on HEAD

postgres=# CREATE SUBSCRIPTION r1015 CONNECTION 'dbname=postgres
host=localhost port=5454' PUBLICATION p WITH (two_phase=1);
NOTICE: created replication slot "r1015" on publisher
CREATE SUBSCRIPTION
postgres=#

--check on 14RC1

postgres=# select two_phase from pg_replication_slots where
slot_name='r105';
two_phase
-----------
f
(1 row)

so are we silently ignoring this parameter as it is not supported on
v14RC/HEAD ? and if yes then why not we just throw an message like
ERROR: unrecognized subscription parameter: "two_phase"

--

There is usually a time lag between a subscription created with two_phase on and
the slot on the publisher enabling two_phase. It only happens after a
tablesync is completed and
the apply worker is restarted. There are logs which indicate that this
has happened. If you could share the
logs (on publisher and subscriber) when this happens, I could have a look.

And in case you do see a problem, I request you create a seperate
thread for this. I didn't want to derail this patch.

regards,
Ajin Cherian
Fujitsu Australia