On Thu, Sep 17, 2026 at 1:13 PM Alexander Korotkov <[email protected]> wrote: > > On Thu, Sep 17, 2026 at 5:03 AM Xuneng Zhou <[email protected]> wrote: > > On Thu, Sep 17, 2026 at 9:43 AM Sami Imseih <[email protected]> wrote: > > > > > > > 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 don't think there is a good use-case here where someone will do: > > > > > > BEGIN ISOLATION LEVEL REPEATBLE READ; WAIT FOR LSN ...; > > > SELECT ... > > > > Can you please elaborate on why this is not a good use-case? > > > > > but, perhaps it's better to be more flexible here. So, I'm ok with > > > proceeding > > > with your suggestion. > > > > +1. One additional pro for this change is to let users have a sense > > for the reason the command is being rejected. A blanket rejection of > > isolation level is somewhat vague since it is an indirect factor. That > > said, If I was a user who is unable or unwilling to read the source > > code and dig into mailing list discussion, I would still be confused > > by the error message and why it is error out. We have detailed commit > > message/doc/comment for the new execution restriction of not holding > > heavyweight lock at the onset of waiting. I am wondering whether it > > could be also helpful to somehow explain the constraints of not > > holding a snapshot for curious users. > > Makes sense. I've added corresponding description to v6. > > > > > 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 .... > > > > This seems sensible to me. > > Done in v6.
Xuneng, Sami, do you think v6 is good enough? ------ Regards, Alexander Korotkov Supabase
