READ ONLY fixes

Attached is a rebased roll-up of the 3 and 3a patches from last month.

-Kevin

On Mon, Jan 10, 2011 at 8:27 AM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Attached is a rebased roll-up of the 3 and 3a patches from last month.
>
> -Kevin

Hi Kevin,

A review:

The main motivation for the patch is to allow future optimization of
read-only transactions, by preventing them from changing back to read
write once their read-onliness has been noticed. This will also
probably bring closer compliance with SQL standards.

The patch was mostly reviewed by others at the time it was proposed and altered.

The patch applies and builds cleanly, and passes make check. It does
what it says.

I found the following message somewhat confusing:
ERROR: read-only property must be set before any query

I was not setting read-only, but rather READ WRITE. This message is
understandable from the perspective of the code (and the "SET
transaction_read_only=..." command), but I think it should be framed
in the context of the SET TRANSACTION command in which read-only is
not the name of a boolean, but one label of a binary switch. Maybe
something like:
ERROR: transaction modes READ WRITE or READ ONLY must be set before any query.

It seems a bit strange that you can do dummy changes (setting the mode
to the same value it currently has) as much as you want in a
subtransaction, but not in a top-level transaction. But this was
discussed previously and not objected to.

The old behavior was not described in the docs. This patch does not
include a doc change, but considering the parallelism between this and
ISOLATION LEVEL, perhaps a parallel sentence should be added to the
docs about this aspect as well.

There are probably many people around who are abusing the current
laxity, so a mention in the release notes is probably warranted.

When a subtransaction has set the mode more stringent than the
top-level transaction did, that setting is reversed when the
subtransaction ends (whether by success or by rollback), which was
discussed as the desired behavior. But the included regression tests
do not exercise that case by testing the case where a SAVEPOINT is
either rolled back or released. Should those tests be included?

The changes made to the isolation level code to get rid of some
spurious warnings are not tested in the regression test--if I excise
that part of the patch, the code still passes make check. Just
looking at that code, it appears to do what it is supposed to, but I
can't figure out how to test it myself.

I poked at the patched code a bit and I could not break it, but I
don't know enough about this part of the system to design truly
devilish tests to apply to it.

I did not do any performance testing, as I don't see how this patch
could have performance implications.

None of the issues I raise above are severe. Does that mean I should
change the status to "ready for committer"?

Cheers,

Jeff

On Sun, Jan 16, 2011 at 6:58 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> None of the issues I raise above are severe.  Does that mean I should
> change the status to "ready for committer"?

Sounds right to me.

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

On Mon, Jan 10, 2011 at 11:27 AM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Attached is a rebased roll-up of the 3 and 3a patches from last month.

Sorry to be a dweeb, but do you have a link to previous discussion?

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

On Sun, Jan 16, 2011 at 6:58 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> I found the following message somewhat confusing:
> ERROR:  read-only property must be set before any query

I think what we need here is two messages, this one and a similar one
that starts with "read-write property...".

> When a subtransaction has set the mode more stringent than the
> top-level transaction did, that setting is reversed when the
> subtransaction ends (whether by success or by rollback), which was
> discussed as the desired behavior.  But the included regression tests
> do not exercise that case by testing the case where a SAVEPOINT is
> either rolled back or released.  Should those tests be included?

+1.

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>> I found the following message somewhat confusing:
>> ERROR: read-only property must be set before any query
>
> I think what we need here is two messages, this one and a similar
> one that starts with "read-write property...".

Done. I started out by being cute with plugging "only" or "write"
into a single message, but then figured that might be hard on
translators; so I went with two separate messages.

Also, I noticed we seemed to be using "transaction read-only mode"
and "transaction read-write mode" elsewhere, so I made this
consistent with the others while I was at it. Hopefully that was a
good idea.

>> When a subtransaction has set the mode more stringent than the
>> top-level transaction did, that setting is reversed when the
>> subtransaction ends (whether by success or by rollback), which
>> was discussed as the desired behavior. But the included
>> regression tests do not exercise that case by testing the case
>> where a SAVEPOINT is either rolled back or released. Should
>> those tests be included?
>
> +1.

Done.

-Kevin

On Fri, Jan 21, 2011 at 7:08 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>>> I found the following message somewhat confusing:
>>> ERROR:  read-only property must be set before any query
>>
>> I think what we need here is two messages, this one and a similar
>> one that starts with "read-write property...".
>
> Done.  I started out by being cute with plugging "only" or "write"
> into a single message, but then figured that might be hard on
> translators; so I went with two separate messages.

Make sense.

I committed the part of this that applies to SET TRANSACTION ISOLATION
LEVEL; the remainder is attached.

Upon further review, I am wondering if it wouldn't be simpler and more
logical to allow idempotent changes of these settings at any time, and
to restrict only changes that actually change something. It feels
really weird to allow changing these properties to their own values at
any time within a subtransaction, but not in a top-level transaction.
Why not:

if (source != PGC_S_OVERRIDE && newval && XactReadOnly)
{
if (IsSubTransaction())
cannot set transaction read-write mode inside a read-only transaction;
else if (FirstSnapshotSet)
transaction read-write mode must be set before any query;
else if (RecoveryInProgress())
cannot set transaction read-write mode during recovery;
}

That seems a lot more straightforward than this logic, and it saves
one translatable message, too.

I'm not bent on this route if people feel strongly otherwise, but it
seems like it'd be simpler without really losing anything.

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> I am wondering if it wouldn't be simpler and more logical to allow
> idempotent changes of these settings at any time, and to restrict
> only changes that actually change something. It feels really
> weird to allow changing these properties to their own values at
> any time within a subtransaction, but not in a top-level
> transaction.

I just looked at the committed code, and saw that it not only
changed things in this regard, but also allows a change from READ
WRITE to READ ONLY at any point in a transaction. (I see now that
your pseudo-code did the same, but I didn't pick up on it at the
time.)

That part of it seems a little weird to me. I think I can live with
it since it doesn't open up any routes to break SSI (now that I
reviewed our use of XactReadOnly and tweaked a function), or to
subvert security except for the unlikely scenario that something is
checking RO state and depending on there having been no writes
earlier in the transaction -- in which case they'd still need to be
very careful about subtransactions.

In short, I'm OK with it but wanted to make sure the community was
aware of the change to what this patch was doing, because I don't
think the discussion made that entirely clear.

-Kevin

On Mon, Jan 24, 2011 at 2:21 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> I am wondering if it wouldn't be simpler and more logical to allow
>> idempotent changes of these settings at any time, and to restrict
>> only changes that actually change something.  It feels really
>> weird to allow changing these properties to their own values at
>> any time within a subtransaction, but not in a top-level
>> transaction.
>
> I just looked at the committed code, and saw that it not only
> changed things in this regard, but also allows a change from READ
> WRITE to READ ONLY at any point in a transaction.  (I see now that
> your pseudo-code did the same, but I didn't pick up on it at the
> time.)
>
> That part of it seems a little weird to me.  I think I can live with
> it since it doesn't open up any routes to break SSI (now that I
> reviewed our use of XactReadOnly and tweaked a function), or to
> subvert security except for the unlikely scenario that something is
> checking RO state and depending on there having been no writes
> earlier in the transaction -- in which case they'd still need to be
> very careful about subtransactions.
>
> In short, I'm OK with it but wanted to make sure the community was
> aware of the change to what this patch was doing, because I don't
> think the discussion made that entirely clear.

Hmm, sorry, I thought that had been made clear. I guess the issue is
that within a subtransaction we can't really prohibit that anyway, so
spending extra code to do it in a toplevel transaction seems like
making the code more complicated just for the heck of it. I wasn't
intending to do anything not agreed...

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