Reject WAIT FOR earlier in transaction-snapshot mode
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253718psql -h localhost -U postgresBuilt from patchset v31 (message #31), September 17, 2026 at 11:39 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253718_31 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253718_31 && git checkout t253718_31Patchset v31 (message #31) is on t253718_31
Hi,
The WAIT FOR docs state:
"WAIT FOR must be executed as a top-level command. It cannot be
executed from a function, procedure, or DO block. It also requires
that no active or registered snapshot be held, and therefore cannot be
used in contexts where such a snapshot must remain active, including
transactions running at isolation levels higher than READ COMMITTED."
So this reads as any transaction above READ COMMITTED should reject WAIT FOR.
However, in REPEATABLE READ and SERIALIZABLE, you are allowed to call
WAIT FOR before taking the snapshot.
```
postgres=# BEGIN ISOLATION LEVEL REPEATABLE READ;
BEGIN
postgres=*# WAIT FOR LSN '0/0' WITH (MODE 'primary_flush');
status
---------
success
(1 row)
postgres=*# select 1;
?column?
----------
1
(1 row)
postgres=*# WAIT FOR LSN '0/0' WITH (MODE 'primary_flush');
ERROR: WAIT FOR must be called without an active or registered snapshot
DETAIL: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED.
postgres=!#
```
That does not match the documented behavior, and I do not think there is
a reason to allow WAIT FOR in such a transaction before the snapshot is
taken.
--
Sami Imseih
Amazon Web Services (AWS)
Hi Sami,
On Wed, Sep 9, 2026 at 3:55 AM Sami Imseih <samimseih.pg@gmail.com> wrote:
Hi,
The WAIT FOR docs state:
"WAIT FOR must be executed as a top-level command. It cannot be
executed from a function, procedure, or DO block. It also requires
that no active or registered snapshot be held, and therefore cannot be
used in contexts where such a snapshot must remain active, including
transactions running at isolation levels higher than READ COMMITTED."So this reads as any transaction above READ COMMITTED should reject WAIT FOR.
However, in REPEATABLE READ and SERIALIZABLE, you are allowed to call
WAIT FOR before taking the snapshot.```
postgres=# BEGIN ISOLATION LEVEL REPEATABLE READ;
BEGIN
postgres=*# WAIT FOR LSN '0/0' WITH (MODE 'primary_flush');
status
---------
success
(1 row)postgres=*# select 1;
?column?
----------
1
(1 row)postgres=*# WAIT FOR LSN '0/0' WITH (MODE 'primary_flush');
ERROR: WAIT FOR must be called without an active or registered snapshot
DETAIL: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED.
postgres=!#
```That does not match the documented behavior, and I do not think there is
a reason to allow WAIT FOR in such a transaction before the snapshot is
taken.
Thanks for the patch! The rationale LGTM.
I've two comments for it:
1) Separate the isolation check from the snapshot check
I am wondering whether it could be helpful to move the isolation level
check to the place after the top level statement checking. It has two
pros -- one is to distinguish isolation levels higher than READ
COMMITTED from READ COMMITTED with snapshots remaining unpopped;
another is to reject early in order to save some parsing effort for
unsupported use.
if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAIT FOR cannot be executed in a transaction with
an isolation level higher than READ COMMITTED")));
2) Valid the wait mode in test
+$node_standby->psql(
+ 'postgres',
+ qq[
+BEGIN ISOLATION LEVEL REPEATABLE READ;
+WAIT FOR LSN '${lsn3}' WITH (MODE 'primary_flush');
+],
Primary flush is not a valid mode in standby, so it will still raise
errors without the patch, though the error message is different. After
the patch, the stability of this test relies on the order of errors
between mode and isolation level checking. It might be helpful to
change the mode to default(replay) like WAIT FOR LSN '${lsn3}';
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
On Wed, Sep 9, 2026 at 3:13 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
Hi Sami,
On Wed, Sep 9, 2026 at 3:55 AM Sami Imseih <samimseih.pg@gmail.com> wrote:
Hi,
The WAIT FOR docs state:
"WAIT FOR must be executed as a top-level command. It cannot be
executed from a function, procedure, or DO block. It also requires
that no active or registered snapshot be held, and therefore cannot be
used in contexts where such a snapshot must remain active, including
transactions running at isolation levels higher than READ COMMITTED."So this reads as any transaction above READ COMMITTED should reject WAIT FOR.
However, in REPEATABLE READ and SERIALIZABLE, you are allowed to call
WAIT FOR before taking the snapshot.```
postgres=# BEGIN ISOLATION LEVEL REPEATABLE READ;
BEGIN
postgres=*# WAIT FOR LSN '0/0' WITH (MODE 'primary_flush');
status
---------
success
(1 row)postgres=*# select 1;
?column?
----------
1
(1 row)postgres=*# WAIT FOR LSN '0/0' WITH (MODE 'primary_flush');
ERROR: WAIT FOR must be called without an active or registered snapshot
DETAIL: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED.
postgres=!#
```That does not match the documented behavior, and I do not think there is
a reason to allow WAIT FOR in such a transaction before the snapshot is
taken.Thanks for the patch! The rationale LGTM.
I've two comments for it:
1) Separate the isolation check from the snapshot check
I am wondering whether it could be helpful to move the isolation level
check to the place after the top level statement checking. It has two
pros -- one is to distinguish isolation levels higher than READ
COMMITTED from READ COMMITTED with snapshots remaining unpopped;
another is to reject early in order to save some parsing effort for
unsupported use.if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAIT FOR cannot be executed in a transaction with
an isolation level higher than READ COMMITTED")));
BEGIN ISOLATION LEVEL READ COMMITTED;
DECLARE c CURSOR FOR SELECT 1;
WAIT FOR LSN '0/0';
-- ERROR: WAIT FOR must be called without an active or registered snapshot
BEGIN ISOLATION LEVEL READ COMMITTED;
SELECT pg_export_snapshot();
WAIT FOR LSN '0/0';
-- ERROR: WAIT FOR must be called without an active or registered snapshot
These are two examples of READ COMMITTED with snapshots remaining unpopped.
2) Valid the wait mode in test
+$node_standby->psql( + 'postgres', + qq[ +BEGIN ISOLATION LEVEL REPEATABLE READ; +WAIT FOR LSN '${lsn3}' WITH (MODE 'primary_flush'); +],Primary flush is not a valid mode in standby, so it will still raise
errors without the patch, though the error message is different. After
the patch, the stability of this test relies on the order of errors
between mode and isolation level checking. It might be helpful to
change the mode to default(replay) like WAIT FOR LSN '${lsn3}';
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
Hi,
Thanks for the review.
BEGIN ISOLATION LEVEL READ COMMITTED;
DECLARE c CURSOR FOR SELECT 1;
WAIT FOR LSN '0/0';
-- ERROR: WAIT FOR must be called without an active or registered snapshotBEGIN ISOLATION LEVEL READ COMMITTED;
SELECT pg_export_snapshot();
WAIT FOR LSN '0/0';
-- ERROR: WAIT FOR must be called without an active or registered snapshot
This means there is a bug in the ERROR message DETAIL. Thanks for pointing
this out.
```
postgres=# begin;
BEGIN
postgres=*# DECLARE c CURSOR FOR SELECT 1;
DECLARE CURSOR
postgres=*# WAIT FOR LSN '0/0' WITH (mode 'PRIMARY_FLUSH');
ERROR: WAIT FOR must be called without an active or registered snapshot
DETAIL: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED.
postgres=!#
```
So I fixed this by first checking the isolation level and using "WAIT
FOR cannot be executed ... READ COMMITTED"
as the errmsg. The existing snapshot check must come after that.
I also added tests to cover the three relevant cases: READ COMMITTED
with a snapshot, and
REPEATABLE READ both with and without a snapshot. The REPEATABLE
READ-with-snapshot
case was already covered, but its expected error message needed to be updated.
The results looks like this:
```
postgres=# BEGIN ISOLATION LEVEL REPEATABLE READ;
BEGIN
postgres=*# select 1;
?column?
----------
1
(1 row)
postgres=*# WAIT FOR LSN '0/0' WITH (mode 'PRIMARY_FLUSH');
ERROR: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED
postgres=!#
```
```
postgres=# BEGIN ISOLATION LEVEL REPEATABLE READ;
BEGIN
postgres=*# WAIT FOR LSN '0/0' WITH (mode 'PRIMARY_FLUSH');
ERROR: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED
postgres=!#
```
```
postgres=# BEGIN;
BEGIN
postgres=*# DECLARE c CURSOR FOR SELECT 1;
DECLARE CURSOR
postgres=*#
postgres=*# WAIT FOR LSN '0/0' WITH (mode 'PRIMARY_FLUSH');
ERROR: WAIT FOR must be called without an active or registered snapshot
postgres=!#
```
--
Sami
On Wed, Sep 9, 2026 at 9:25 PM Sami Imseih <samimseih.pg@gmail.com> wrote:
Hi,
Thanks for the review.
BEGIN ISOLATION LEVEL READ COMMITTED;
DECLARE c CURSOR FOR SELECT 1;
WAIT FOR LSN '0/0';
-- ERROR: WAIT FOR must be called without an active or registered snapshotBEGIN ISOLATION LEVEL READ COMMITTED;
SELECT pg_export_snapshot();
WAIT FOR LSN '0/0';
-- ERROR: WAIT FOR must be called without an active or registered snapshotThis means there is a bug in the ERROR message DETAIL. Thanks for pointing
this out.```
postgres=# begin;
BEGIN
postgres=*# DECLARE c CURSOR FOR SELECT 1;
DECLARE CURSOR
postgres=*# WAIT FOR LSN '0/0' WITH (mode 'PRIMARY_FLUSH');
ERROR: WAIT FOR must be called without an active or registered snapshot
DETAIL: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED.
postgres=!#
```So I fixed this by first checking the isolation level and using "WAIT
FOR cannot be executed ... READ COMMITTED"
as the errmsg. The existing snapshot check must come after that.I also added tests to cover the three relevant cases: READ COMMITTED
with a snapshot, and
REPEATABLE READ both with and without a snapshot. The REPEATABLE
READ-with-snapshot
case was already covered, but its expected error message needed to be updated.The results looks like this:
```
postgres=# BEGIN ISOLATION LEVEL REPEATABLE READ;
BEGIN
postgres=*# select 1;
?column?
----------
1
(1 row)postgres=*# WAIT FOR LSN '0/0' WITH (mode 'PRIMARY_FLUSH');
ERROR: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED
postgres=!#
``````
postgres=# BEGIN ISOLATION LEVEL REPEATABLE READ;
BEGIN
postgres=*# WAIT FOR LSN '0/0' WITH (mode 'PRIMARY_FLUSH');
ERROR: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED
postgres=!#
``````
postgres=# BEGIN;
BEGIN
postgres=*# DECLARE c CURSOR FOR SELECT 1;
DECLARE CURSOR
postgres=*#
postgres=*# WAIT FOR LSN '0/0' WITH (mode 'PRIMARY_FLUSH');
ERROR: WAIT FOR must be called without an active or registered snapshot
postgres=!#
V2 LGTM. Do you prefer to keep the snapshot and isolation-level checks
next to each other?
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
V2 LGTM.
Thanks!
Do you prefer to keep the snapshot and isolation-level checks
next to each other?
Can you elaborate on this point? Do you mean combine them in
a single block?
--
Sami
On Wed, Sep 9, 2026 at 10:11 PM Sami Imseih <samimseih.pg@gmail.com> wrote:
V2 LGTM.
Thanks!
Do you prefer to keep the snapshot and isolation-level checks
next to each other?Can you elaborate on this point? Do you mean combine them in
a single block?
1) Separate the isolation check from the snapshot check
I am wondering whether it could be helpful to move the isolation level
check to the place after the top level statement checking. It has two
pros -- one is to distinguish isolation levels higher than READ
COMMITTED from READ COMMITTED with snapshots remaining unpopped;
another is to reject early in order to save some parsing effort for
unsupported use.
I might not express this clearly in the first email I replied
upthread. Currently, we pop and invalidate the snapshot after the
grammar parsing and argument validation. That placement is sensible,
but the inverse order of them seems not, because it is not meaningful
to clean-up snapshots for already invalid inputs and it also invites
confusion for error reporting if snapshot and input handling both went
wrong. However, moving the new separate isolation-level check to the
very beginning of the function immediately after the existing
top-level statement check somehow make sense to me:
/*
* WAIT FOR must not be run as a non-top-level statement (e.g., inside a
* function, procedure, or DO block). Forbid this case upfront.
*/
if (!isTopLevel)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("%s can only be executed as a top-level statement",
"WAIT FOR"),
errdetail("WAIT FOR cannot be used within a function, procedure, or DO
block.")));
The rationale for this move is to reject unsupported use early so as
to save some parsing/invalidation effort. The downside of this is that
it scatters the logic of snapshot management, which is the core of all
the existing restrictions like running as top-level statements under
selected isolation levels. That said, we already make the rejection of
non-top-level statements early in the function, which means they are
not clustered even for now...
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
Hi,
Not critically important but note that the PopActiveSnapshot and
InvalidateCatalogSnapshot call and the comment about "giving up if there
is still an active or registered snapshot" have a close relationship; I
think it would make sense to keep them together. I would add your new
test-and-ereport just before the PopActiveSnapshot() call, to avoid
breaking that.
Cheers
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)
The rationale for this move is to reject unsupported use early so as
to save some parsing/invalidation effort. The downside of this is that
it scatters the logic of snapshot management, which is the core of all
the existing restrictions like running as top-level statements under
selected isolation levels. That said, we already make the rejection of
non-top-level statements early in the function, which means they are
not clustered even for now...
I guess it can be moved, but then instead of what we currently see
```
postgres=# begin isolation level repeatable read ; WAIT FOR LSN
'11110/111111111';
BEGIN
ERROR: invalid input syntax for type pg_lsn: "11110/111111111"
postgres=!#
```
we will get
```
postgres=# begin isolation level repeatable read ; WAIT FOR LSN
'11110/111111111';
BEGIN
ERROR: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED
postgres=!#
```
It is arguable which one is better here, but I am inclined to prefer
parsing errors first. They tell the user immediately that the supplied
LSN is invalid. Also, the parsing here is very cheap, so I do not think
there is much benefit in trying to reject earlier just to save that work.
Of course, the existing !isTopLevel restriction behaves differently here,
since it fires before we parse the WAIT FOR arguments at all. If I were
inclined to change anything else, I would move both the top-level and
isolation-level checks to just after parsing but before
This way for top-level we get:
```
postgres=# DO $$
BEGIN
EXECUTE 'WAIT FOR LSN ''111111111/11''';
END
$$;
ERROR: invalid input syntax for type pg_lsn: "111111111/11"
CONTEXT: SQL statement "WAIT FOR LSN '111111111/11'"
PL/pgSQL function inline_code_block line 3 at EXECUTE
postgres=#
```
instead of
```
postgres=# DO $$
BEGIN
EXECUTE 'WAIT FOR LSN ''0/0''';
END
$$;
ERROR: WAIT FOR can only be executed as a top-level statement
DETAIL: WAIT FOR cannot be used within a function, procedure, or DO block.
CONTEXT: SQL statement "WAIT FOR LSN '0/0'"
PL/pgSQL function inline_code_block line 3 at EXECUTE
postgres=#
```
but not sure that's worth it either.
--
Sami
On Wed, Sep 9, 2026 at 11:42 PM Sami Imseih <samimseih.pg@gmail.com> wrote:
The rationale for this move is to reject unsupported use early so as
to save some parsing/invalidation effort. The downside of this is that
it scatters the logic of snapshot management, which is the core of all
the existing restrictions like running as top-level statements under
selected isolation levels. That said, we already make the rejection of
non-top-level statements early in the function, which means they are
not clustered even for now...I guess it can be moved, but then instead of what we currently see
```
postgres=# begin isolation level repeatable read ; WAIT FOR LSN
'11110/111111111';
BEGIN
ERROR: invalid input syntax for type pg_lsn: "11110/111111111"
postgres=!#
```we will get
```
postgres=# begin isolation level repeatable read ; WAIT FOR LSN
'11110/111111111';
BEGIN
ERROR: WAIT FOR cannot be executed within a transaction with an
isolation level higher than READ COMMITTED
postgres=!#
```It is arguable which one is better here, but I am inclined to prefer
parsing errors first. They tell the user immediately that the supplied
LSN is invalid. Also, the parsing here is very cheap, so I do not think
there is much benefit in trying to reject earlier just to save that work.
Yes, this is more about which error fires first. The error-message
style guide only says to give users clear, factual explanations; it
does not state a universal precedence for execution precondition and
argument validation. For empirical reference, I asked Astra to do an
in-tree analysis of the patterns of precedents. Here's what it found:
-- VACUUM
Parse and validate options → check transaction/nested-execution
restrictions → later release the active snapshot
-- REINDEX CONCURRENTLY
Validate option values → check transaction restrictions → resolve the
tablespace name
-- CREATE DATABASE
Check transaction restrictions → validate command options, including
numeric values
-- ALTER SYSTEM
Check transaction restrictions → enter the handler and validate the setting
-- SET TRANSACTION SNAPSHOT
Check transaction state → check isolation level → validate the
snapshot identifier
Both patterns exist in the tree. I am ok with leaving the
isolation-level check and top-level statement check where they are.
Of course, the existing !isTopLevel restriction behaves differently here,
since it fires before we parse the WAIT FOR arguments at all. If I were
inclined to change anything else, I would move both the top-level and
isolation-level checks to just after parsing but beforeThis way for top-level we get:
```
postgres=# DO $$
BEGIN
EXECUTE 'WAIT FOR LSN ''111111111/11''';
END
$$;
ERROR: invalid input syntax for type pg_lsn: "111111111/11"
CONTEXT: SQL statement "WAIT FOR LSN '111111111/11'"
PL/pgSQL function inline_code_block line 3 at EXECUTE
postgres=#
```
instead of```
postgres=# DO $$
BEGIN
EXECUTE 'WAIT FOR LSN ''0/0''';
END
$$;
ERROR: WAIT FOR can only be executed as a top-level statement
DETAIL: WAIT FOR cannot be used within a function, procedure, or DO block.
CONTEXT: SQL statement "WAIT FOR LSN '0/0'"
PL/pgSQL function inline_code_block line 3 at EXECUTE
postgres=#
```but not sure that's worth it either.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
Hi Álvaro,
On Thu, Sep 10, 2026 at 6:21 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
Hi,
Not critically important but note that the PopActiveSnapshot and
InvalidateCatalogSnapshot call and the comment about "giving up if there
is still an active or registered snapshot" have a close relationship; I
think it would make sense to keep them together. I would add your new
test-and-ereport just before the PopActiveSnapshot() call, to avoid
breaking that.
Good catch. +1 for doing that.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
Not critically important but note that the PopActiveSnapshot and
InvalidateCatalogSnapshot call and the comment about "giving up if there
is still an active or registered snapshot" have a close relationship; I
think it would make sense to keep them together. I would add your new
test-and-ereport just before the PopActiveSnapshot() call, to avoid
breaking that.Good catch. +1 for doing that.
Sure, that works. See the attached.
I think as this is an error reporting bug, maybe we should backpatch to 19?
Sami Imseih
Amazon Web Services (AWS)
Attachments:
t253718_12v3-0001-Fix-WAIT-FOR-rejection-errors.patchapplication/octet-stream; name=v3-0001-Fix-WAIT-FOR-rejection-errors.patchDownload+31-5
On Fri, Sep 11, 2026 at 1:40 AM Sami Imseih <samimseih.pg@gmail.com> wrote:
Not critically important but note that the PopActiveSnapshot and
InvalidateCatalogSnapshot call and the comment about "giving up if there
is still an active or registered snapshot" have a close relationship; I
think it would make sense to keep them together. I would add your new
test-and-ereport just before the PopActiveSnapshot() call, to avoid
breaking that.Good catch. +1 for doing that.
Sure, that works. See the attached.
Thanks.
I think as this is an error reporting bug, maybe we should backpatch to 19?
+1 for backpatch. The patch suffices for that purpose, since there's
no divergence between v19 and HEAD for this feature currently. For
cautiousness, I also asked for an AI review for this patch. Here's
what it suggested:
1) Could we use 0/0 instead of $lsn3 for these rejection tests? Since
$lsn3 is deliberately unreachable, the first test can hang if the
isolation check is missing.
2) Also, the isolation-error pattern matches the old DETAIL, so
matching the ERROR: prefix would verify that it is now the primary
error. The cursor case better additionally check that the misleading
isolation-level detail is absent.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
1) Could we use 0/0 instead of $lsn3 for these rejection tests? Since
$lsn3 is deliberately unreachable, the first test can hang if the
isolation check is missing.
I think I will keep this as-is. If the isolation-level rejection is
missing, the test is broken.
2) Also, the isolation-error pattern matches the old DETAIL, so
matching the ERROR: prefix would verify that it is now the primary
error.
v4 tightens the new recovery tests so the REPEATABLE READ cases match
the primary ERROR line, rather than the old DETAIL text. I also cleaned
up one test description.
The cursor case better additionally check that the misleading
isolation-level detail is absent.
I don't think we need that. The cursor case only needs to verify the
new primary error.
--
Sami Imseih
Amazon Web Services (AWS)
Attachments:
t253718_14v4-0001-Fix-WAIT-FOR-rejection-errors.patchapplication/octet-stream; name=v4-0001-Fix-WAIT-FOR-rejection-errors.patchDownload+34-8
On Fri, Sep 11, 2026 at 11:58 PM Sami Imseih <samimseih.pg@gmail.com> wrote:
1) Could we use 0/0 instead of $lsn3 for these rejection tests? Since
$lsn3 is deliberately unreachable, the first test can hang if the
isolation check is missing.I think I will keep this as-is. If the isolation-level rejection is
missing, the test is broken.2) Also, the isolation-error pattern matches the old DETAIL, so
matching the ERROR: prefix would verify that it is now the primary
error.v4 tightens the new recovery tests so the REPEATABLE READ cases match
the primary ERROR line, rather than the old DETAIL text. I also cleaned
up one test description.The cursor case better additionally check that the misleading
isolation-level detail is absent.I don't think we need that. The cursor case only needs to verify the
new primary error.
Thanks. I am OK with your judgement!
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
On Sat, Sep 12, 2026 at 10:48 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Fri, Sep 11, 2026 at 11:58 PM Sami Imseih <samimseih.pg@gmail.com> wrote:
1) Could we use 0/0 instead of $lsn3 for these rejection tests? Since
$lsn3 is deliberately unreachable, the first test can hang if the
isolation check is missing.I think I will keep this as-is. If the isolation-level rejection is
missing, the test is broken.2) Also, the isolation-error pattern matches the old DETAIL, so
matching the ERROR: prefix would verify that it is now the primary
error.v4 tightens the new recovery tests so the REPEATABLE READ cases match
the primary ERROR line, rather than the old DETAIL text. I also cleaned
up one test description.The cursor case better additionally check that the misleading
isolation-level detail is absent.I don't think we need that. The cursor case only needs to verify the
new primary error.Thanks. I am OK with your judgement!
Here's a rebase due to a23ab4862cf.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
Attachments:
t253718_16v5-0001-Fix-WAIT-FOR-rejection-errors.patchapplication/octet-stream; name=v5-0001-Fix-WAIT-FOR-rejection-errors.patchDownload+35-8
[RMT hat]
Please note that this is now listed as an open item for v19. Alexander,
please give us a status update when you're able.
--
nathan
Hi!
On Wed, Sep 16, 2026 at 4:28 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Sat, Sep 12, 2026 at 10:48 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Fri, Sep 11, 2026 at 11:58 PM Sami Imseih <samimseih.pg@gmail.com> wrote:
1) Could we use 0/0 instead of $lsn3 for these rejection tests? Since
$lsn3 is deliberately unreachable, the first test can hang if the
isolation check is missing.I think I will keep this as-is. If the isolation-level rejection is
missing, the test is broken.2) Also, the isolation-error pattern matches the old DETAIL, so
matching the ERROR: prefix would verify that it is now the primary
error.v4 tightens the new recovery tests so the REPEATABLE READ cases match
the primary ERROR line, rather than the old DETAIL text. I also cleaned
up one test description.The cursor case better additionally check that the misleading
isolation-level detail is absent.I don't think we need that. The cursor case only needs to verify the
new primary error.Thanks. I am OK with your judgement!
Here's a rebase due to a23ab4862cf.
I went through this thread. I don't quite agree with the proposed
patch. I think construction like
BEGIN ISOLATION LEVEL ... ; WAIT FOR LSN ...; do the work
should work at any isolation level. v4 patch makes it work with READ
COMMITTED, but fail with higher levels. I think this is undesired
asymmetry.
I suggest we should better fix the docs. The attached v5 patch does
it. Additionally it makes errdetail() about the isolation level
conditional and shown only on isolation levels higher than READ
COMMITTED.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
t253718_18v5-0001-Say-what-WAIT-s-snapshot-restriction-actually-is.patchapplication/octet-stream; name=v5-0001-Say-what-WAIT-s-snapshot-restriction-actually-is.patchDownload+30-6
Hi, Nathan!
On Wed, Sep 16, 2026 at 6:50 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
[RMT hat]
Please note that this is now listed as an open item for v19. Alexander,
please give us a status update when you're able.
Thank you. I put my hands on it [1]. Will resolve this ASAP.
Links.
1. /messages/by-id/CAPpHfdv1zWu1ma_SexGLBN_35+cYbJ5SfbdauvjwCPGE-9TyBw@mail.gmail.com
------
Regards,
Alexander Korotkov
Supabase
I went through this thread. I don't quite agree with the proposed
patch. I think construction likeBEGIN ISOLATION LEVEL ... ; WAIT FOR LSN ...; do the work
should work at any isolation level. v4 patch makes it work with READ
COMMITTED, but fail with higher levels. I think this is undesired
asymmetry.
I don't think there is a good use-case here where someone will do:
BEGIN ISOLATION LEVEL REPEATBLE READ; WAIT FOR LSN ...;
SELECT ...
but, perhaps it's better to be more flexible here. So, I'm ok with proceeding
with your suggestion.
I suggest we should better fix the docs. The attached v5 patch does
it. Additionally it makes errdetail() about the isolation level
conditional and shown only on isolation levels higher than READ
COMMITTED.
The existing error message is:
```
errmsg("WAIT must be called without an active or registered snapshot")
```
and v5 uses the same terminology in the documentation:
```
<command>DO</command> block. It also requires that no active or
registered snapshot be held.
```
The terms "active snapshot" and "registered snapshot" describe internal
snapshot states, and don't seem appropriate for user-facing docs/error
messages.
For example, the COPY documentation instead refers to "snapshots held
by this transaction".
Could we instead use:
```
errmsg("WAIT cannot be executed while the current transaction
holds a snapshot")
```
and change the documentation to:
```
<command>DO</command> block. It also cannot be executed while the
current transaction holds a snapshot. A transaction running at an
isolation level higher than <literal>READ COMMITTED</literal> holds
one from its first query until it ends ....
```
--
Sami Imseih
Amazon Web Services (AWS)