On Wed, Sep 9, 2026 at 11:42 PM Sami Imseih <[email protected]> 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 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. -- Regards, Xuneng Zhou HighGo Software Co., Ltd.
