pgsql: Implement waiting for given lsn at transaction start
Implement waiting for given lsn at transaction start
This commit adds following optional clause to BEGIN and START TRANSACTION
commands.
WAIT FOR LSN lsn [ TIMEOUT timeout ]
New clause pospones transaction start till given lsn is applied on standby.
This clause allows user be sure, that changes previously made on primary would
be visible on standby.
New shared memory struct is used to track awaited lsn per backend. Recovery
process wakes up backend once required lsn is applied.
Author: Ivan Kartyshov, Anna Akenteva
Reviewed-by: Craig Ringer, Thomas Munro, Robert Haas, Kyotaro Horiguchi
Reviewed-by: Masahiko Sawada, Ants Aasma, Dmitry Ivanov, Simon Riggs
Reviewed-by: Amit Kapila, Alexander Korotkov
Discussion: /messages/by-id/0240c26c-9f84-30ea-fca9-93ab2df5f305@postgrespro.ru
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/0f5ca02f53ac2b211d8518f0882c49284c0c9610
Modified Files
--------------
doc/src/sgml/ref/begin.sgml | 17 +-
doc/src/sgml/ref/start_transaction.sgml | 17 +-
src/backend/access/transam/xlog.c | 13 ++
src/backend/commands/Makefile | 3 +-
src/backend/commands/wait.c | 295 ++++++++++++++++++++++++++++++++
src/backend/nodes/copyfuncs.c | 15 ++
src/backend/nodes/equalfuncs.c | 13 ++
src/backend/nodes/outfuncs.c | 28 +++
src/backend/parser/gram.y | 37 +++-
src/backend/storage/ipc/ipci.c | 7 +
src/backend/storage/lmgr/proc.c | 4 +
src/backend/tcop/utility.c | 13 ++
src/backend/utils/adt/misc.c | 2 -
src/include/commands/wait.h | 26 +++
src/include/nodes/nodes.h | 1 +
src/include/nodes/parsenodes.h | 12 ++
src/include/parser/kwlist.h | 3 +
src/include/utils/timestamp.h | 2 +
src/test/recovery/t/020_begin_wait.pl | 85 +++++++++
src/tools/pgindent/typedefs.list | 2 +
20 files changed, 585 insertions(+), 10 deletions(-)
Alexander Korotkov <akorotkov@postgresql.org> writes:
Implement waiting for given lsn at transaction start
This commit adds following optional clause to BEGIN and START TRANSACTION
commands.
WAIT FOR LSN lsn [ TIMEOUT timeout ]
This seems like a really carelessly chosen syntax --- *three* new
keywords, when you probably didn't need any. Are you not aware that
there is distributed overhead in the grammar for every keyword?
Plus, each new keyword carries the risk of breaking existing
applications, since it no longer works as an alias-not-preceded-by-AS.
I have no particular opinion on the value of the feature, but I wish
a different syntax had been chosen.
regards, tom lane
On Tue, Apr 7, 2020 at 2:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alexander Korotkov <akorotkov@postgresql.org> writes:
Implement waiting for given lsn at transaction start
This commit adds following optional clause to BEGIN and START TRANSACTION
commands.
WAIT FOR LSN lsn [ TIMEOUT timeout ]This seems like a really carelessly chosen syntax --- *three* new
keywords, when you probably didn't need any. Are you not aware that
there is distributed overhead in the grammar for every keyword?
Plus, each new keyword carries the risk of breaking existing
applications, since it no longer works as an alias-not-preceded-by-AS.I have no particular opinion on the value of the feature, but I wish
a different syntax had been chosen.
I was curious whether the syntax got this kind of discussion, followed the
discussion link, and the last email seen reads:
"""
Worse, it was marked Needs Review even though no new patch was provided.
I'm going to set this back to Returned with Feedback. If anyone has a
good
reason that it should be in the CF we can always revive it.
+1.
"""
While there is lots of discussion it ended up with the thread at "returned
with feedback" (a month ago) and now we have a commit. There seems to be
other relevant discussion not linked to.
David J.
On 2020-04-08 00:45, David G. Johnston wrote:
While there is lots of discussion it ended up with the thread at
"returned with feedback" (a month ago) and now we have a commit.
There seems to be other relevant discussion not linked to.David J.
Hello! Sorry about the confusion.
This feature somehow managed to have multiple separate discussion
threads:
[1]: /messages/by-id/0240c26c-9f84-30ea-fca9-93ab2df5f305@postgrespro.ru
/messages/by-id/0240c26c-9f84-30ea-fca9-93ab2df5f305@postgrespro.ru
[2]: /messages/by-id/80f267591b373db5588d580fdfb432f0@postgrespro.ru
/messages/by-id/80f267591b373db5588d580fdfb432f0@postgrespro.ru
[3]: /messages/by-id/195e2d07ead315b1620f1a053313f490@postgrespro.ru
/messages/by-id/195e2d07ead315b1620f1a053313f490@postgrespro.ru
The latest discussion was happening in [3]/messages/by-id/195e2d07ead315b1620f1a053313f490@postgrespro.ru.
--
Anna Akenteva
Postgres Professional:
The Russian Postgres Company
http://www.postgrespro.com
On Wed, Apr 8, 2020 at 1:34 AM Anna Akenteva <a.akenteva@postgrespro.ru> wrote:
On 2020-04-08 00:45, David G. Johnston wrote:
While there is lots of discussion it ended up with the thread at
"returned with feedback" (a month ago) and now we have a commit.
There seems to be other relevant discussion not linked to.David J.
Hello! Sorry about the confusion.
This feature somehow managed to have multiple separate discussion
threads:
[1]
/messages/by-id/0240c26c-9f84-30ea-fca9-93ab2df5f305@postgrespro.ru
[2]
/messages/by-id/80f267591b373db5588d580fdfb432f0@postgrespro.ru
[3]
/messages/by-id/195e2d07ead315b1620f1a053313f490@postgrespro.ru
My email client somehow merge these threads into single one. This is
why I missed [2] and [3] in the commit message. Sorry for that.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Wed, Apr 8, 2020 at 12:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alexander Korotkov <akorotkov@postgresql.org> writes:
Implement waiting for given lsn at transaction start
This commit adds following optional clause to BEGIN and START TRANSACTION
commands.
WAIT FOR LSN lsn [ TIMEOUT timeout ]This seems like a really carelessly chosen syntax --- *three* new
keywords, when you probably didn't need any. Are you not aware that
there is distributed overhead in the grammar for every keyword?
I had theoretical knowledge about that, but I didn't manage to apply it.
Plus, each new keyword carries the risk of breaking existing
applications, since it no longer works as an alias-not-preceded-by-AS.
I wasn't aware about this. This is a good point, which I will remember.
I have no particular opinion on the value of the feature, but I wish
a different syntax had been chosen.
Sure, we'll prepare an update for syntax soon.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On 4/7/20 7:32 PM, Alexander Korotkov wrote:
On Wed, Apr 8, 2020 at 1:34 AM Anna Akenteva <a.akenteva@postgrespro.ru> wrote:
On 2020-04-08 00:45, David G. Johnston wrote:
While there is lots of discussion it ended up with the thread at
"returned with feedback" (a month ago) and now we have a commit.
There seems to be other relevant discussion not linked to.David J.
Hello! Sorry about the confusion.
This feature somehow managed to have multiple separate discussion
threads:
[1]
/messages/by-id/0240c26c-9f84-30ea-fca9-93ab2df5f305@postgrespro.ru
[2]
/messages/by-id/80f267591b373db5588d580fdfb432f0@postgrespro.ru
[3]
/messages/by-id/195e2d07ead315b1620f1a053313f490@postgrespro.ruMy email client somehow merge these threads into single one. This is
why I missed [2] and [3] in the commit message. Sorry for that.
Well, I think Ivan should have certainly replied on the original thread
(where I marked the patch RwF after Fujii's recommendation) before
detaching it and reviving the patch.
Better yet, the original thread should not have been detached at all.
Now having read the active thread it seems this patch was pushed through
at the last moment despite some objections from Amit.
I can see there are new proposals for syntax post-commit on the thread.
Honestly, I'm at a loss for what to say. This just seems wrong.
Regards,
--
-David
david@pgmasters.net
On Wed, Apr 8, 2020 at 5:34 AM David Steele <david@pgmasters.net> wrote:
On 4/7/20 7:32 PM, Alexander Korotkov wrote:
On Wed, Apr 8, 2020 at 1:34 AM Anna Akenteva <a.akenteva@postgrespro.ru> wrote:
On 2020-04-08 00:45, David G. Johnston wrote:
While there is lots of discussion it ended up with the thread at
"returned with feedback" (a month ago) and now we have a commit.
There seems to be other relevant discussion not linked to.David J.
Hello! Sorry about the confusion.
This feature somehow managed to have multiple separate discussion
threads:
[1]
/messages/by-id/0240c26c-9f84-30ea-fca9-93ab2df5f305@postgrespro.ru
[2]
/messages/by-id/80f267591b373db5588d580fdfb432f0@postgrespro.ru
[3]
/messages/by-id/195e2d07ead315b1620f1a053313f490@postgrespro.ruMy email client somehow merge these threads into single one. This is
why I missed [2] and [3] in the commit message. Sorry for that.Well, I think Ivan should have certainly replied on the original thread
(where I marked the patch RwF after Fujii's recommendation) before
detaching it and reviving the patch.Better yet, the original thread should not have been detached at all.
Now having read the active thread it seems this patch was pushed through
at the last moment despite some objections from Amit.
Yeah, I was nervous about the syntax as according to what I read in
the thread there was not a broad consensus on it. That's why
yesterday, I asked opinions from others even though I knew it is late
for PG13 to get feedback.
I can see there are new proposals for syntax post-commit on the thread.
Honestly, I'm at a loss for what to say. This just seems wrong.
I think we have two options now (a) Provide feedback on the thread for
syntax and see what best we can do in that regard (b) Revert and try
it for PG14. I think generally building consensus on syntax at this
stage is difficult but we can try if we want this feature for this
release. I am not very happy that it went in without more discussion
but OTOH, this is not a very big feature and if we agree on syntax
this can be part of PG13. I think code also needs some more review.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Wed, Apr 08, 2020 at 06:07:23AM +0530, Amit Kapila wrote:
I think we have two options now (a) Provide feedback on the thread for
syntax and see what best we can do in that regard (b) Revert and try
it for PG14. I think generally building consensus on syntax at this
stage is difficult but we can try if we want this feature for this
release. I am not very happy that it went in without more discussion
but OTOH, this is not a very big feature and if we agree on syntax
this can be part of PG13. I think code also needs some more review.
I have not followed this stuff as much as I would have liked, but my
overall impression of the patch is that the disagreements around the
grammar syntax with the addition of three keywords which I guess are
heavily used in AS aliases because they are generic terms (including
lsn!), and the issues mentioned on the thread point out that this
patch was not ready to be merged, and that this has been rushed as per
the feature freeze deadline. Post feature-freeze is not the time to
discuss feature redesign, so I think that this should be reverted, and
proposed again for 14.
--
Michael
On Wed, Apr 8, 2020 at 5:18 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Apr 08, 2020 at 06:07:23AM +0530, Amit Kapila wrote:
I think we have two options now (a) Provide feedback on the thread for
syntax and see what best we can do in that regard (b) Revert and try
it for PG14. I think generally building consensus on syntax at this
stage is difficult but we can try if we want this feature for this
release. I am not very happy that it went in without more discussion
but OTOH, this is not a very big feature and if we agree on syntax
this can be part of PG13. I think code also needs some more review.I have not followed this stuff as much as I would have liked, but my
overall impression of the patch is that the disagreements around the
grammar syntax with the addition of three keywords which I guess are
heavily used in AS aliases because they are generic terms (including
lsn!), and the issues mentioned on the thread point out that this
patch was not ready to be merged, and that this has been rushed as per
the feature freeze deadline. Post feature-freeze is not the time to
discuss feature redesign, so I think that this should be reverted, and
proposed again for 14.
In my original idea, we can assume simplified syntax to be agreed.
And other issues could be easily resolved if arise. But it appears we
have serious complains to even simplified syntax. For sure, we
shouldn't discuss it post feature freeze. I've reverted this commit.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Wed, Apr 08, 2020 at 11:45:35AM +0300, Alexander Korotkov wrote:
In my original idea, we can assume simplified syntax to be agreed.
And other issues could be easily resolved if arise. But it appears we
have serious complains to even simplified syntax. For sure, we
shouldn't discuss it post feature freeze. I've reverted this commit.
Thanks, Alexander.
--
Michael