Wrong command name in writeable-CTE related error messages
Hi!
I noticed that errors due to writable CTEs in read-only or non-volatile context say the offensive command is SELECT.
For example a writeable CTE in a IMMUTABLE function:
CREATE TABLE t (x INTEGER);
CREATE FUNCTION immutable_func()
RETURNS INTEGER
LANGUAGE SQL
IMMUTABLE
AS $$
WITH x AS (
INSERT INTO t (x) VALUES (1) RETURNING x
) SELECT * FROM x;
$$;
SELECT immutable_func();
ERROR: SELECT is not allowed in a non-volatile function
Or a writeable CTE in read-only transaction:
START TRANSACTION READ ONLY;
WITH x AS (
INSERT INTO t (x) VALUES (1) RETURNING x
)
SELECT * FROM x;
ERROR: cannot execute SELECT in a read-only transaction
My first thought was that these error messages should mention INSERT, but after looking into the source I’m not sure anymore. The name of the command is obtained from CreateCommandName(). After briefly looking around it doesn’t seem to be trivial to introduce something along the line of CreateModifyingCommandName().
So I started by using a different error message at those places where I think it should. I’ve attached a patch for reference, but I’m not happy with it. In particular I’m unsure about the SPI stuff (how to test?) and if there are more cases as those covered by the patch. Ultimately getting hold of the command name might also be beneficial for a new error message.
A WITH clause containing a data-modifying statement is not allowed in a read-only transaction
It wouldn’t make me sad if somebody who touches the code more often than once every few years can take care of it.
-markus
Attachments:
WIP-writable_cte_error_message-001.patchapplication/octet-stream; name=WIP-writable_cte_error_message-001.patch; x-unix-mode=0644Download+17-9
Markus Winand <markus.winand@winand.at> writes:
I noticed that errors due to writable CTEs in read-only or non-volatile context say the offensive command is SELECT.
Good point.
My first thought was that these error messages should mention INSERT, but after looking into the source I’m not sure anymore. The name of the command is obtained from CreateCommandName(). After briefly looking around it doesn’t seem to be trivial to introduce something along the line of CreateModifyingCommandName().
Yeah, you would have to inspect the plan tree pretty carefully to
determine that.
Given the way the test is written, maybe it'd make sense to forget about
mentioning the command name, and instead identify the table we are
complaining about:
ERROR: table "foo" cannot be modified in a read-only transaction
I don't see any huge point in using PreventCommandIfReadOnly if we
go that way, so no refactoring is needed: just test XactReadOnly
directly.
regards, tom lane
On 23.05.2023, at 19:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Markus Winand <markus.winand@winand.at> writes:
I noticed that errors due to writable CTEs in read-only or non-volatile context say the offensive command is SELECT.
Good point.
My first thought was that these error messages should mention INSERT, but after looking into the source I’m not sure anymore. The name of the command is obtained from CreateCommandName(). After briefly looking around it doesn’t seem to be trivial to introduce something along the line of CreateModifyingCommandName().
Yeah, you would have to inspect the plan tree pretty carefully to
determine that.Given the way the test is written, maybe it'd make sense to forget about
mentioning the command name, and instead identify the table we are
complaining about:ERROR: table "foo" cannot be modified in a read-only transaction
Attached patch takes the active form:
cannot modify table ”foo" in a read-only transaction
It obtains the table name by searching rtable for an RTE_RELATION with rellockmode == RowExclusiveLock. Not sure if there are any cases where that falls apart.
I don't see any huge point in using PreventCommandIfReadOnly if we
go that way, so no refactoring is needed: just test XactReadOnly
directly.
As there are several places where this is needed, the patch introduces some utility functions.
More interestingly, I found that BEGIN ATOMIC bodies of non-volatile functions happily accept data-modifying statements and FOR UPDATE. While they fail at runtime it was my expectation that this would be caught at CREATE time. The attached patch also takes care of this by walking the Query tree and looking for resultRelation and hasForUpdate — assuming that non-volatile functions should have neither. Let me know if this is desired behavior or not.
-markus