Обсуждение: not fully correct error message

On Thu, Jan 1, 2026 at 3:10 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
> I tested one use case, and maybe I found little bit possible error message
>
> create procedure test()
> as $$
> begin
>   vacuum;
> end;
> $$ language plpgsql;
>
> (2026-01-01 08:04:05) postgres=# call test();
> ERROR:  25001: VACUUM cannot be executed from a function
> CONTEXT:  SQL statement "vacuum"
> PL/pgSQL function test() line 3 at SQL statement
> LOCATION:  PreventInTransactionBlock, xact.c:3695
> (2026-01-01 08:09:18) postgres=#
>
> should be "VACUUM cannot be executed from a function or a procedure" instead ?
>

hi.
"VACUUM cannot be executed from a function or a procedure"
looks good to me.

similarly, in ExecWaitStmt we have:
        ereport(ERROR,
                errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                errmsg("WAIT FOR must be only called without an active
or registered snapshot"),
                errdetail("WAIT FOR cannot be executed from a function
or a procedure or within a transaction with an isolation level higher
than READ COMMITTED."));

PreventInTransactionBlock is used in so many places, but this error message:
``
(errmsg("%s cannot be executed from a function", stmtType)));
``
only appears once in the regress tests.
maybe we can add some dummy tests for it.

On 1/3/26 7:34 AM, Pavel Stehule wrote:
> here is a patch (with small regress test)

Looks like a good change to me since the obvious question many people 
would ask themselves after seeing this error is if running it in a 
procedure would work instead and it does not. And this error message 
changes saves those people time which is a great thing.

Tried out this patch and it worked and, as expected, also improved the 
error for other commands like CREATE DATABASE and REINDEX CONCURRENTLY 
so when this is commit the commit message should make it clear this is 
not just about VACUUM.

A small suggestion is to change the message from:

"%s cannot be executed from a function or a procedure"

to:

"%s cannot be executed from a function or procedure"

I am not a native speaker but I think the second flows better.

Andreas

On 1/3/26 1:22 PM, Marcos Pegoraro wrote:
> Em sáb., 3 de jan. de 2026 às 03:35, Pavel Stehule 
> <pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>> escreveu:
> 
>     here is a patch (with small regress test)
> 
> 
> An anonymous block doesn't accept vacuum too.
> Wouldn't it be better to specify what kind of block you are running 
> instead of
> function, procedure or anonymous block ?

Maybe out of some kind of correctness, but it seems less useful to me 
since the obvious question an end user would ask after trying to run 
VACUUM in a function is if they can do so in a procedure instead so we 
may as well tell them right away.

A potential third option would be to take your solution but to add a 
HINT about that it needs to run as a top-level statement outside any 
transactions, but I kinda liked how simple the original patch was.

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> On 1/3/26 1:22 PM, Marcos Pegoraro wrote:
>> Em sáb., 3 de jan. de 2026 às 03:35, Pavel Stehule
>> <pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>> escreveu:
>>> here is a patch (with small regress test)

>> An anonymous block doesn't accept vacuum too.
>> Wouldn't it be better to specify what kind of block you are running
>> instead of function, procedure or anonymous block ?

I don't think PreventInTransactionBlock has ready access to that
information.

> A potential third option would be to take your solution but to add a
> HINT about that it needs to run as a top-level statement outside any
> transactions, but I kinda liked how simple the original patch was.

Yeah, I like just adding "or procedure" and calling it good.
I do not think we need a regression test, either ...

Poking around, I also found this:

src/backend/commands/wait.c:                              errdetail("WAIT FOR cannot be executed from a function or a
procedureor within a transaction with an isolation level higher than READ COMMITTED.")); 

which is also not great grammar.  What do you think of "WAIT FOR
cannot be executed from a function or procedure, nor within a
transaction with an isolation level higher than READ COMMITTED." ?

            regards, tom lane

On 1/3/26 7:03 PM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> A potential third option would be to take your solution but to add a
>> HINT about that it needs to run as a top-level statement outside any
>> transactions, but I kinda liked how simple the original patch was.
> 
> Yeah, I like just adding "or procedure" and calling it good.
> I do not think we need a regression test, either ...

Yeah, let's keep it simple.

> Poking around, I also found this:
> 
> src/backend/commands/wait.c:                              errdetail("WAIT FOR cannot be executed from a function or a
procedureor within a transaction with an isolation level higher than READ COMMITTED."));
 
> 
> which is also not great grammar.  What do you think of "WAIT FOR
> cannot be executed from a function or procedure, nor within a
> transaction with an isolation level higher than READ COMMITTED." ?

Much better!

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> On 1/3/26 7:03 PM, Tom Lane wrote:
>> Yeah, I like just adding "or procedure" and calling it good.
>> I do not think we need a regression test, either ...

> Yeah, let's keep it simple.

>> Poking around, I also found this:
>> src/backend/commands/wait.c:                              errdetail("WAIT FOR cannot be executed from a function or
aprocedure or within a transaction with an isolation level higher than READ COMMITTED.")); 
>> which is also not great grammar.  What do you think of "WAIT FOR
>> cannot be executed from a function or procedure, nor within a
>> transaction with an isolation level higher than READ COMMITTED." ?

> Much better!

Putting that all together, and fixing affected regression tests
(yes, this code was covered already), I get the attached.

            regards, tom lane

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 842faa44232..c857e23552f 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3695,7 +3695,8 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
         ereport(ERROR,
                 (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
         /* translator: %s represents an SQL statement name */
-                 errmsg("%s cannot be executed from a function", stmtType)));
+                 errmsg("%s cannot be executed from a function or procedure",
+                        stmtType)));

     /* If we got past IsTransactionBlock test, should be in default state */
     if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
diff --git a/src/backend/commands/wait.c b/src/backend/commands/wait.c
index d43dfd642d6..e4509fffe06 100644
--- a/src/backend/commands/wait.c
+++ b/src/backend/commands/wait.c
@@ -131,8 +131,8 @@ ExecWaitStmt(ParseState *pstate, WaitStmt *stmt, DestReceiver *dest)
     if (HaveRegisteredOrActiveSnapshot())
         ereport(ERROR,
                 errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                errmsg("WAIT FOR must be only called without an active or registered snapshot"),
-                errdetail("WAIT FOR cannot be executed from a function or a procedure or within a transaction with an
isolationlevel higher than READ COMMITTED.")); 
+                errmsg("WAIT FOR must be called without an active or registered snapshot"),
+                errdetail("WAIT FOR cannot be executed from a function or procedure, nor within a transaction with an
isolationlevel higher than READ COMMITTED.")); 

     /*
      * As the result we should hold no snapshot, and correspondingly our xmin
diff --git a/src/test/recovery/t/049_wait_for_lsn.pl b/src/test/recovery/t/049_wait_for_lsn.pl
index e0ddb06a2f0..5f415b9af51 100644
--- a/src/test/recovery/t/049_wait_for_lsn.pl
+++ b/src/test/recovery/t/049_wait_for_lsn.pl
@@ -102,7 +102,7 @@ $node_standby->psql(
     "BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT 1; WAIT FOR LSN '${lsn3}';",
     stderr => \$stderr);
 ok( $stderr =~
-      /WAIT FOR must be only called without an active or registered snapshot/,
+      /WAIT FOR must be called without an active or registered snapshot/,
     "get an error when running in a transaction with an isolation level higher than REPEATABLE READ"
 );

@@ -122,7 +122,7 @@ $node_standby->psql(
     "SELECT pg_wal_replay_wait_wrap('${lsn3}');",
     stderr => \$stderr);
 ok( $stderr =~
-      /WAIT FOR must be only called without an active or registered snapshot/,
+      /WAIT FOR must be called without an active or registered snapshot/,
     "get an error when running within another function");

 # 5. Check parameter validation error cases on standby before promotion
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 327d1e7731f..b3eccd8afe3 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -357,7 +357,7 @@ END;
 CREATE FUNCTION func() RETURNS VOID AS
 $$ ALTER SUBSCRIPTION regress_testsub SET PUBLICATION mypub WITH (refresh = true) $$ LANGUAGE SQL;
 SELECT func();
-ERROR:  ALTER SUBSCRIPTION with refresh cannot be executed from a function
+ERROR:  ALTER SUBSCRIPTION with refresh cannot be executed from a function or procedure
 CONTEXT:  SQL function "func" statement 1
 ALTER SUBSCRIPTION regress_testsub DISABLE;
 ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);

On 1/3/26 8:16 PM, Tom Lane wrote:
> Putting that all together, and fixing affected regression tests
> (yes, this code was covered already), I get the attached.

That looks good to me.

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> On 1/3/26 8:16 PM, Tom Lane wrote:
>> Putting that all together, and fixing affected regression tests
>> (yes, this code was covered already), I get the attached.

> That looks good to me.

Pushed.

            regards, tom lane