Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Flexible options for CREATE_REPLICATION_SLOT.

  2. Flexible options for BASE_BACKUP.

  1. extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2020-06-11T20:40:54Z

    Hi,

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

This version of the patch does not include documentation, but
hopefully it's fairly clear from reading the code what it's all about.
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. At some point in the distant
future we could retire the old syntax, when we've stopped caring about
compatibility with pre-14 versions.

Thoughts?

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

  2. Re: extensible options syntax for replication parser?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-06-14T07:15:37Z

    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.

  3. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2020-06-24T15:51:24Z

    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

  4. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2020-10-08T14:33:06Z

    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

  5. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-10T19:44:22Z

    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

  6. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-21T16:27:21Z

    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
http://postgr.es/m/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

  7. Re: extensible options syntax for replication parser?

    Sergei Kornilov <sk@zsrv.org> — 2021-09-22T12:11:10Z

    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

  8. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-22T19:55:47Z

    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

  9. Re: extensible options syntax for replication parser?

    tushar <tushar.ahuja@enterprisedb.com> — 2021-09-23T06:54:57Z

    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

  10. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-23T15:05:40Z

    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

  11. Re: extensible options syntax for replication parser?

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-09-24T04:01:18Z

    
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

  12. Re: extensible options syntax for replication parser?

    tushar <tushar.ahuja@enterprisedb.com> — 2021-09-24T11:28:06Z

    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

  13. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-24T16:55:00Z

    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

  14. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-24T17:06:30Z

    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

  15. Re: extensible options syntax for replication parser?

    tushar <tushar.ahuja@enterprisedb.com> — 2021-09-24T17:54:27Z

    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

  16. Re: extensible options syntax for replication parser?

    tushar <tushar.ahuja@enterprisedb.com> — 2021-09-24T18:27:59Z

    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

  17. Re: extensible options syntax for replication parser?

    tushar <tushar.ahuja@enterprisedb.com> — 2021-09-24T18:38:19Z

    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

  18. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-24T18:38:53Z

    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

  19. Re: extensible options syntax for replication parser?

    Ajin Cherian <itsajin@gmail.com> — 2021-09-27T01:20:12Z

    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

  20. Re: extensible options syntax for replication parser?

    Ajin Cherian <itsajin@gmail.com> — 2021-09-27T03:59:46Z

    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

  21. Re: extensible options syntax for replication parser?

    tushar <tushar.ahuja@enterprisedb.com> — 2021-09-27T07:11:14Z

    On 9/25/21 12:08 AM, Robert Haas wrote:
> 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.

Right, issue is reproducible on HEAD as well. I should have checked 
that, sorry about it.

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

  22. Re: extensible options syntax for replication parser?

    tushar <tushar.ahuja@enterprisedb.com> — 2021-09-27T07:15:12Z

    On 9/27/21 9:29 AM, Ajin Cherian wrote:
> 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.

It would be great if we throw an error rather than silently ignoring 
this parameter ,  I opened a separate email for this

https://www.postgresql.org/message-id/CAC6VRoY3SAFeO7kZ0EOVC6mX%3D1ZyTocaecTDTh209W20KCC_aQ%40mail.gmail.com

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

  23. Re: extensible options syntax for replication parser?

    Robert Haas <robertmhaas@gmail.com> — 2021-10-05T16:56:12Z

    On Mon, Sep 27, 2021 at 3:15 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> It would be great if we throw an error rather than silently ignoring
> this parameter ,  I opened a separate email for this
>
> https://www.postgresql.org/message-id/CAC6VRoY3SAFeO7kZ0EOVC6mX%3D1ZyTocaecTDTh209W20KCC_aQ%40mail.gmail.com

Hearing no further comments, I've gone ahead and committed these
patches. I'm still slightly nervous that I may have missed some issue,
but I think at this point having the patches in the tree is more
likely to turn it up than any other course of action.

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

  24. Re: extensible options syntax for replication parser?

    tushar <tushar.ahuja@enterprisedb.com> — 2021-10-13T14:05:47Z

    On 10/5/21 10:26 PM, Robert Haas wrote:
> Hearing no further comments, I've gone ahead and committed these
> patches. I'm still slightly nervous that I may have missed some issue,
> but I think at this point having the patches in the tree is more
> likely to turn it up than any other course of action.
I have tested couple of scenarios of pg_basebackup / pg_receivewal 
/pg_recvlogical /  Publication(wal_level=logical) and
Subscription e.t.c against HEAD (with patches) and  cross-version 
testing. Things look good to me and no breakage was found.

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