Обсуждение: perplexing error message

There are a lot of things that are understandably forbidden in a
read-only transaction, but one would not expect SELECT to be among
them.  And yet, one can get the system to complain about precisely
that:

rhaas=# create table rules_src(f1 int, f2 int);
ERROR:  relation "rules_src" already exists
rhaas=# create table rules_log(f1 int, f2 int, tag text);
ERROR:  relation "rules_log" already exists
rhaas=# insert into rules_src values(1,2), (11,12);
INSERT 0 2
rhaas=# create rule r2 as on update to rules_src do also
rhaas-# values(old.*, 'old'), (new.*, 'new');
ERROR:  rule "r2" for relation "rules_src" already exists
rhaas=# begin transaction read only;
BEGIN
rhaas=# update rules_src set f2 = f2 / 10;
ERROR:  cannot execute SELECT in a read-only transaction

It sees fair for this to fail; I am after all attempting an update
inside of a read-only transaction.  But it is mighty strange to
complain about SELECT, since (1) the example contains exactly 0
instances of the keyword SELECT and (2) SELECT is a read-only
operation.  Changing "do also" to "do instead" produces the same
failure.  This seems to be the result of this code in
ExecCheckXactReadOnly:
       if ((rte->requiredPerms & (~ACL_SELECT)) == 0)           continue;

...
       PreventCommandIfReadOnly(CreateCommandTag((Node *) plannedstmt));

There's nothing obviously stupid about that, but the results in this
case don't make much sense.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> There are a lot of things that are understandably forbidden in a
> read-only transaction, but one would not expect SELECT to be among
> them.  And yet, one can get the system to complain about precisely
> that:
> ... This seems to be the result of this code in ExecCheckXactReadOnly:

>         if ((rte->requiredPerms & (~ACL_SELECT)) == 0)
>             continue;
> ...
>         PreventCommandIfReadOnly(CreateCommandTag((Node *) plannedstmt));

> There's nothing obviously stupid about that, but the results in this
> case don't make much sense.

What's going on here is that we're checking the SELECT that is added
by the rule, and per TFM that executes before the UPDATE, so you get
this error first rather than one complaining about UPDATE (which you
would get later if this one hadn't interfered).

It's visible at this level that this PlannedStmt isn't the main query
(because it has canSetTag = 0), but that does not help much because
we have no very convenient way to find the main querytree.  Anyway
you can construct related scenarios where the tag for the current
PlannedStmt probably *is* the thing to use, so I'm disinclined to
try to fix it by locating the main querytree to use the tag for that.

I think a more realistic way to fix it is to adjust rule expansion so that
the RTE generated for rules_src in the added query is marked with only the
privileges required for that query --- I imagine right now, we're just
copying the requiredPerms verbatim from the original query.  There's
certainly no other obvious reason why an RTE in a SELECT (VALUES)
querytree would have the ACL_UPDATE bit set, as this one does.
        regards, tom lane

On Sat, Feb 7, 2015 at 4:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What's going on here is that we're checking the SELECT that is added
> by the rule, and per TFM that executes before the UPDATE, so you get
> this error first rather than one complaining about UPDATE (which you
> would get later if this one hadn't interfered).
>
> It's visible at this level that this PlannedStmt isn't the main query
> (because it has canSetTag = 0), but that does not help much because
> we have no very convenient way to find the main querytree.  Anyway
> you can construct related scenarios where the tag for the current
> PlannedStmt probably *is* the thing to use, so I'm disinclined to
> try to fix it by locating the main querytree to use the tag for that.
>
> I think a more realistic way to fix it is to adjust rule expansion so that
> the RTE generated for rules_src in the added query is marked with only the
> privileges required for that query --- I imagine right now, we're just
> copying the requiredPerms verbatim from the original query.  There's
> certainly no other obvious reason why an RTE in a SELECT (VALUES)
> querytree would have the ACL_UPDATE bit set, as this one does.

That seems to be what's happening.  Specifically, rewriteRuleAction() does this:
   sub_action->rtable = list_concat((List *) copyObject(parsetree->rtable),
sub_action->rtable);

There's a long comment justifying that; the important point, as far as
I can see, is that when we expand a view we want to check permissions
on the view, not the expansion.  In other words, this is fairly
intentional behavior.

Now, we could reduce the scope of the weirdness by clearing all the
required permission bits from the copied range table in the DO-ALSO
case, because we know there is a copy with the original permission
bits floating around someplace and so those bits will be checked.  But
that's not a complete fix, because you'll still get an error
complaining about SELECT if you try to UPDATE a view with a DO INSTEAD
SELECT rule.  In fact, I'm not really sure why that case should fail
at all.  Despite the fact that we require UPDATE permission to perform
it in this instance, it's still a SELECT at heart, and that's a
read-only operation.  Maybe we should be keying
ExecCheckXactReadOnly's decision off of the PlannedStmt's commandType;
if not CMD_SELECT, then iterate over resultRelations and check whether
each of those is an RTE_RELATION in a non-temp namespace.

I'm not sure that's correct when hasModifyingCTE is set, though.  I
don't know what to do about that case.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company