Обсуждение: Read-only transactions

I would like to implement read-only transactions following the SQL spec,
so we can check off this item on the supported list.  According to the
list I gathered, the following commands will fail if the transaction is
read-only:

alter *
analyze
checkpoint
cluster
comment
create *
delete (from non-temporary table)
drop *
execute (if the prepared statement is one in the list)
explain analyze (ditto)
grant
insert (into non-temporary table)
reindex
revoke
select into
truncate
update (non-temporary table)
vacuum

I think it's light-weight and marginally useful.

-- 
Peter Eisentraut   peter_e@gmx.net

Peter Eisentraut <peter_e@gmx.net> writes:
> I would like to implement read-only transactions following the SQL spec,
> ...
> I think it's light-weight and marginally useful.

"Light-weight" would depend on your intended implementation, I suppose.
Where are you planning to check this?

Also, the fact that you are excluding temp tables seems to suggest that
this is a very high-level, abstract notion of read-only-ness; it's
certainly got little to do with whether we try to write on the disk.
As such it's not clear to me why vacuum and checkpoint are included in
the forbidden list.  They don't logically change any data.  The same
might be said of reindex.
        regards, tom lane

Tom Lane writes:

> Where are you planning to check this?

In general, I'm trying to align it like a (self-imposed) permission check.
For the query-like statements I'm looking at ExecCheckRTPerms().  (That
also handles EXECUTE and EXPLAIN most easily.)  Utility statements have a
check in tcop/utility.c, COPY does it in DoCopy() (out of convenience).
In any case you don't pay more than a 'if (XactReadOnly && ...)' if it's
not activated.

> As such it's not clear to me why vacuum and checkpoint are included in
> the forbidden list.  They don't logically change any data.  The same
> might be said of reindex.

You're right.  I'll allow that class of statements.

-- 
Peter Eisentraut   peter_e@gmx.net

Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane writes:
>> Where are you planning to check this?

> In general, I'm trying to align it like a (self-imposed) permission check.
> For the query-like statements I'm looking at ExecCheckRTPerms().  (That
> also handles EXECUTE and EXPLAIN most easily.)

If you put it there then EXPLAIN UPDATE ... will bomb out.  EXPLAIN
ANALYZE UPDATE *should* bomb out, but it'd be nice not to for the other
case.  Not sure if it's worth kluging things to make that happen, though.
The executor doesn't currently know the difference between EXPLAIN and
EXPLAIN ANALYZE.

> Utility statements have a
> check in tcop/utility.c, COPY does it in DoCopy() (out of convenience).
> In any case you don't pay more than a 'if (XactReadOnly && ...)' if it's
> not activated.

Yeah, one if-test per statement isn't much overhead.  What I'm more
worried about is making sure that all the places that need to check it
will check it; particularly in the utility-statement area, we shall
surely be adding more and more things that need to check it.

If it's done in ProcessUtility for utility statements then it's probably
fairly hard to miss for new statements.  May I suggest that each
case branch that does not need to check it include an explicit comment to
that effect, eg.
       case T_VacuumStmt:           /* No XactReadOnly check since this logically changes no data */
vacuum((VacuumStmt*) parsetree);           break;
 

Then it'll be hard to miss the need to think about this when adding a
new statement.
        regards, tom lane

Tom Lane writes:

>         case T_VacuumStmt:
>             /* No XactReadOnly check since this logically changes no data */
>             vacuum((VacuumStmt *) parsetree);
>             break;
>
> Then it'll be hard to miss the need to think about this when adding a
> new statement.

Well, I had one big check at the top so it doesn't have to be spread out
so much:

static void
check_xact_readonly(Node *parsetree)
{if (!XactReadOnly)    return;
switch (nodeTag(parsetree)){    case T_AlterDatabaseSetStmt:    case T_AlterDomainStmt:    case T_AlterGroupStmt:
[...]   case T_DropUserStmt:    case T_GrantStmt:    case T_TruncateStmt:        elog(ERROR, "transaction is
read-only");       break;}
 
}

-- 
Peter Eisentraut   peter_e@gmx.net