Thread: 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: https://postgr.es/m/0240c26c-9f84-30ea-fca9-93ab2df5f305%40postgrespro.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] https://postgr.es/m/0240c26c-9f84-30ea-fca9-93ab2df5f305%40postgrespro.ru [2] https://postgr.es/m/80f267591b373db5588d580fdfb432f0%40postgrespro.ru [3] https://postgr.es/m/195e2d07ead315b1620f1a053313f490%40postgrespro.ru The latest discussion was happening in [3]. -- 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] > https://postgr.es/m/0240c26c-9f84-30ea-fca9-93ab2df5f305%40postgrespro.ru > [2] > https://postgr.es/m/80f267591b373db5588d580fdfb432f0%40postgrespro.ru > [3] > https://postgr.es/m/195e2d07ead315b1620f1a053313f490%40postgrespro.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] >> https://postgr.es/m/0240c26c-9f84-30ea-fca9-93ab2df5f305%40postgrespro.ru >> [2] >> https://postgr.es/m/80f267591b373db5588d580fdfb432f0%40postgrespro.ru >> [3] >> https://postgr.es/m/195e2d07ead315b1620f1a053313f490%40postgrespro.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. 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] > >> https://postgr.es/m/0240c26c-9f84-30ea-fca9-93ab2df5f305%40postgrespro.ru > >> [2] > >> https://postgr.es/m/80f267591b373db5588d580fdfb432f0%40postgrespro.ru > >> [3] > >> https://postgr.es/m/195e2d07ead315b1620f1a053313f490%40postgrespro.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. > > 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
Attachment
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