Re: READ ONLY fixes - Mailing list pgsql-hackers

On Mon, Jan 10, 2011 at 8:27 AM, Kevin Grittner
<Kevin.Grittner@wicourts.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