Re: serializable read only deferrable

Tom Lane < tgl(at)sss(dot)pgh(dot)pa(dot)us > wrote:
> Hmm. This patch disallows the case of creating a read-only
> subtransaction of a read-write parent. That's a step backwards.
> I'm not sure how we could enforce that the property not change
> after the first query of a subxact, but maybe we don't care that
> much? Do your optimizations pay attention to local read-only in a
> subxact?

No, it's all about the top level transaction, as long as the
subtransaction doesn't do anything which violates the requirements
of the top level. (That is, if the top level is not READ ONLY, I
can't do the optimizations, but it would cause no problem if a
subtransaction was READ ONLY -- it just wouldn't allow any special
optimizations.)

I noticed that the standard seems (if I'm reading it correctly) to
allow subtransactions to switch to more restrictive settings for
both transaction isolation and read only status than the enclosing
transaction, but not looser. I don't think it makes sense in
PostgreSQL to say (for example) that the top level transaction is
READ COMMITTED but the subtransaction is SERIALIZABLE, but it might
make sense to say that the top level transaction is READ WRITE but
the subtransaction is READ ONLY. And I see where I broke support
for that in the patch.

I can fix up the patch if to support it again if you like. (I think
it's just a matter of replacing a few lines that I replaced in the
original patch.) If you'd rather do it, I'll stay out of your way.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> I noticed that the standard seems (if I'm reading it correctly) to
> allow subtransactions to switch to more restrictive settings for
> both transaction isolation and read only status than the enclosing
> transaction, but not looser.

Yeah. My recollection is that we've discussed exactly this point with
respect to isolation level, and decided that we couldn't (or it wasn't
worthwhile to) allow serializable subxacts inside repeatable read.
I don't know whether your patch will change that tradeoff. But I don't
think it's really been discussed much with respect to read-only, perhaps
because nobody's paid all that much attention to read-only at all.
In any case, the behavior you state seems obviously correct, so let's
see what we can do about getting closer to that.

My guess is that a reasonable fix is to remember the read-only setting
as of snapshot lockdown, and thereafter to allow changing from
read-write to read-only but not vice versa. One thing to watch for
is allowing subxact exit to restore the previous read-write state.
(BTW it looks like assign_XactIsoLevel emits a rather useless debug
message in that case, so that code could stand some cleanup too. Also,
that code is set so that it will throw an error even if you're assigning
the currently active setting, which maybe is overly strict? Not sure.)

> I can fix up the patch if to support it again if you like. (I think
> it's just a matter of replacing a few lines that I replaced in the
> original patch.) If you'd rather do it, I'll stay out of your way.

Feel free to have a go at it.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:

> One thing to watch for is allowing subxact exit to restore the
> previous read-write state.

OK.

> BTW it looks like assign_XactIsoLevel emits a rather useless debug
> message in that case, so that code could stand some cleanup too.

I'll take a look.

> Also, that code is set so that it will throw an error even if
> you're assigning the currently active setting, which maybe is
> overly strict? Not sure.

The standard is tricky to read, but my reading of it is that only
"LOCAL" changes are allowed after the transaction is underway (which
I *think* effectively means a subtransaction), and those can't make
the setting less strict -- you're allowed to specify the same level
or more strict. There would be no harm from the perspective of
anything I'm working on to allow an in-progress transaction to be
set to what it already has, but that seems to invite confusion and
error more than provide a helpful feature, as far as I can tell.
I'm inclined not to allow it except at the start of a
subtransaction, but don't feel strongly about it.

>> I can fix up the patch if to support it again if you like.

> Feel free to have a go at it.

Will do. I notice that I also removed the check for
RecoveryInProgress(), which I will also restore. And maybe a
regression test or two for the subtransaction usages....

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Also, that code is set so that it will throw an error even if
>> you're assigning the currently active setting, which maybe is
>> overly strict? Not sure.

> The standard is tricky to read, but my reading of it is that only
> "LOCAL" changes are allowed after the transaction is underway (which
> I *think* effectively means a subtransaction), and those can't make
> the setting less strict -- you're allowed to specify the same level
> or more strict. There would be no harm from the perspective of
> anything I'm working on to allow an in-progress transaction to be
> set to what it already has, but that seems to invite confusion and
> error more than provide a helpful feature, as far as I can tell.
> I'm inclined not to allow it except at the start of a
> subtransaction, but don't feel strongly about it.

Hmm ...

(1) If the standard says that you're allowed to apply a redundant setting,
I think we'd better accept that.

(2) I'm not thrilled about the idea of tracking the equivalent of
FirstSnapshotSet for each subtransaction, which I think would be
necessary infrastructure to error-check this as tightly as you seem to
have in mind. I'd prefer to be a bit laxer in order to have less
overhead for what is in the end just nanny-ism. So the implementation
I had in mind would allow SET TRANSACTION operations to occur later in a
subxact, as long as they were redundant and weren't actually trying to
change the active value.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> If the standard says that you're allowed to apply a redundant
> setting, I think we'd better accept that.

OK

> So the implementation I had in mind would allow SET TRANSACTION
> operations to occur later in a subxact, as long as they were
> redundant and weren't actually trying to change the active value.

It's easy to see how I can allow changes in the subtransaction as
long as they don't specify READ WRITE when the top level is READ
ONLY, but it isn't obvious to me how to only allow it at the start
of the subtransaction. I'm OK with taking the easy route on this
aspect of things, but if someone needs READ ONLY to "stick" for the
duration of a subtransaction, I'm not sure how to do that. (And I'm
not sure you were actually suggesting that, either.)

To restate, since I'm not sure how clear that is, what I have at the
moment is:

(1) A top level transaction can only set READ ONLY or READ WRITE
until it has acquired its first snapshot.

(2) A subtransaction can set it at will, as many times as desired,
to match the top level or specify READ ONLY.

(3) During recovery the setting cannot be changed from READ ONLY to
READ WRITE.

I'm not clear whether #2 is OK or how to do it only at the start.
I haven't yet looked at the other issues you mentioned.

-Kevin

On Dec8, 2010, at 20:39 , Kevin Grittner wrote:
> The standard is tricky to read, but my reading of it is that only
> "LOCAL" changes are allowed after the transaction is underway (which
> I *think* effectively means a subtransaction), and those can't make
> the setting less strict -- you're allowed to specify the same level
> or more strict. There would be no harm from the perspective of
> anything I'm working on to allow an in-progress transaction to be
> set to what it already has, but that seems to invite confusion and
> error more than provide a helpful feature, as far as I can tell.
> I'm inclined not to allow it except at the start of a
> subtransaction, but don't feel strongly about it.

Hm, I think being able to assert that the isolation level really is SERIALIZABLE by simply doing "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE" would be a great feature for SSI.

Say you've written a trigger which enforces some complex constraint, but is correct only for SERIALIZABLE transactions. By simply sticking a "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE" at the top of the trigger you'd both document that fact it is correct only for SERIALIZABLE transactions *and* prevent corruption should the isolation level be something else due to a pilot error. Nice, simply and quite effective.

BTW, I hope to find some time this evening to review your more detailed proposal for "serializable read only deferrable"

best regards,
Florian Pflug

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> So the implementation I had in mind would allow SET TRANSACTION
>> operations to occur later in a subxact, as long as they were
>> redundant and weren't actually trying to change the active value.

> It's easy to see how I can allow changes in the subtransaction as
> long as they don't specify READ WRITE when the top level is READ
> ONLY, but it isn't obvious to me how to only allow it at the start
> of the subtransaction. I'm OK with taking the easy route on this
> aspect of things, but if someone needs READ ONLY to "stick" for the
> duration of a subtransaction, I'm not sure how to do that. (And I'm
> not sure you were actually suggesting that, either.)

What I suggested was to not allow read-only -> read-write state
transitions except (1) before first snapshot in the main xact
and (2) at subxact exit (the OVERRIDE case). That seems to accomplish
the goal. Now it will also allow dropping down to read-only
mid-subtransaction, but I don't think forbidding that case is worth
extra complexity.

regards, tom lane

Florian Pflug <fgp(at)phlo(dot)org> wrote:

> Hm, I think being able to assert that the isolation level really
> is SERIALIZABLE by simply doing "SET TRANSACTION ISOLATION LEVEL
> SERIALIZABLE" would be a great feature for SSI.
>
> Say you've written a trigger which enforces some complex
> constraint, but is correct only for SERIALIZABLE transactions. By
> simply sticking a "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
> at the top of the trigger you'd both document that fact it is
> correct only for SERIALIZABLE transactions *and* prevent
> corruption should the isolation level be something else due to
> a pilot error. Nice, simply and quite effective.

It would be great to have a way within a trigger, or possibly other
functions, to assert that the transaction isolation level is
serializable. What gives me pause here is that the standard allows
you to specify a more strict transaction isolation level within a
subtransaction without error, so this way of spelling the feature is
flirting with rather nonstandard behavior.

Is there maybe a better way to check this?

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Florian Pflug <fgp(at)phlo(dot)org> wrote:
>> Say you've written a trigger which enforces some complex
>> constraint, but is correct only for SERIALIZABLE transactions. By
>> simply sticking a "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
>> at the top of the trigger you'd both document that fact it is
>> correct only for SERIALIZABLE transactions *and* prevent
>> corruption should the isolation level be something else due to
>> a pilot error. Nice, simply and quite effective.

> It would be great to have a way within a trigger, or possibly other
> functions, to assert that the transaction isolation level is
> serializable. What gives me pause here is that the standard allows
> you to specify a more strict transaction isolation level within a
> subtransaction without error, so this way of spelling the feature is
> flirting with rather nonstandard behavior.

Yes. This is not the way to provide a feature like that.

> Is there maybe a better way to check this?

You can always read the current setting and throw an error if you
don't like it.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> What I suggested was to not allow read-only -> read-write state
> transitions except (1) before first snapshot in the main xact
> and (2) at subxact exit (the OVERRIDE case). That seems to
> accomplish the goal. Now it will also allow dropping down to
> read-only mid-subtransaction, but I don't think forbidding that
> case is worth extra complexity.

Attached is version 2. I think it does everything we discussed,
except that I'm not sure whether I addressed the assign_XactIsoLevel
issue you mentioned, since you mentioned a debug message and I only
see things that look like errors to me. If I did miss something,
I'll be happy to take another look if you can point me to the right
place.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> except that I'm not sure whether I addressed the assign_XactIsoLevel
> issue you mentioned, since you mentioned a debug message and I only
> see things that look like errors to me. If I did miss something,
> I'll be happy to take another look if you can point me to the right
> place.

GUC_complaint_elevel() can return DEBUGn, and in fact will do so in
the PGC_S_OVERRIDE case. I'm thinking that the code ought to look
more like

/* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
if (source != PGC_S_OVERRIDE)
{
check and report all the complaint-worthy cases;
}

The original coding was probably sane for initial development, but
now it just results in useless debug-log traffic for predictable
perfectly valid cases.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> GUC_complaint_elevel() can return DEBUGn, and in fact will do so in
> the PGC_S_OVERRIDE case. I'm thinking that the code ought to look
> more like
>
> /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort
*/
> if (source != PGC_S_OVERRIDE)
> {
> check and report all the complaint-worthy cases;
> }

Done. Version 3 attached. I think I've covered all bases now.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> Done. Version 3 attached.

My final tweaks didn't make it into that diff. Attached is 3a as a
patch over the version 3 patch.

-Kevin