Re: SSI patch version 14

Jeff Davis wrote:

> 1. In CheckForSerializableConflictIn(), I think the comment above
> may be out of date. It says:

> 2. Also in the comment above CheckForSerializableConflictIn(), I
> see:

> 3. The comment above CheckForSerializableConflictOut() seems to
> trail off, as though you may have meant to write more. It also
> seems to be out of date.

Will fix and post a patch version 15, along with the other fixes
based on feedback to v14 (mostly to comments and docs) within a few
hours.

I'll also do that global change from using "tran" as an abbreviation
for transaction in some places to consistently using xact whenever it
is abbreviated.

> And why are you reading the infomask directly? Do the existing
> visibility functions not suffice?

It's possible we re-invented some code somewhere, but I'm not clear
on what code from this patch might use what existing function. Could
you provide specifics?

> I have made it through predicate.c, and I have a much better
> understanding of what it's actually doing. I can't claim that I
> have a clear understanding of everything involved, but the code
> looks like it's in good shape (given the complexity involved) and
> well-commented.

Thanks! I know that's a lot of work, and I appreciate you pointing
out where comments have not kept up with coding.

> I am marking the patch Ready For Committer, because any committer
> will need time to go through the patch; and the authors have
> clearly applied the thought, care, and testing required for
> something of this complexity. I will continue to work on it,
> though.

Thanks!

> The biggest issue on my mind is what to do about Hot Standby. The
> authors have a plan, but there is also some resistance to it:
>
>
http://archives.postgresql.org/message-id/23698.1295566621@sss.pgh.pa.us
>
> We don't need a perfect solution for 9.1, but it would be nice if
> we had a viable plan for 9.2.

I don't recall any real opposition to what I sketched out in this
post, which came after the above-referenced one:

http://archives.postgresql.org/message-id/4D39D5EC0200002500039A19@gw.wicourts.gov

Also, that opposition appears to be based on a misunderstanding of
the first alternative, which was for sending at most one snapshot per
commit or rollback of a serializable read write transaction, with
possible throttling. The alternative needs at most two bits per
commit or rollback of a serializable read write transaction; although
I haven't checked whether that can be scared up without adding a
whole byte. Read only transactions have nothing to do with the
traffic under either alternative.

-Kevin

On Mon, 2011-01-31 at 07:26 -0600, Kevin Grittner wrote:
> > And why are you reading the infomask directly? Do the existing
> > visibility functions not suffice?
>
> It's possible we re-invented some code somewhere, but I'm not clear
> on what code from this patch might use what existing function. Could
> you provide specifics?

In CheckForSerializableConflictOut(), it takes a boolean "valid". Then
within the function, it tries to differentiate:

1. Valid with no indication that it will be deleted.
2. Valid, but delete in progress
3. Invalid

For #1, you are using the hint bit (not the real transaction status),
and manually checking whether it's just a lock or a real delete. For #2
you are assuming any other xmax means that the transaction is in
progress (which it may not be, because the hint bit might not be set for
some time). I assume that will cause additional false positives.

If you used HeapTupleSatisfiesVacuum(), you could do something like:

case HEAPTUPLE_LIVE:
return;
case HEAPTUPLE_RECENTLY_DEAD:
case HEAPTUPLE_DELETE_IN_PROGRESS:
xid = HeapTupleHeaderGetXmax(tuple->t_data);
break;
case HEAPTUPLE_INSERT_IN_PROGRESS:
xid = HeapTupleHeaderGetXmin(tuple->t_data);
break;
case HEAPTUPLE_DEAD:
return;

This is not identical to what's happening currently, and I haven't
thought this through thoroughly yet. For instance, "recently dead and
invalid" would be checking on the xmax instead of the xmin. Perhaps you
could exit early in that case (if you still keep the "valid" flag), but
that will happen soon enough anyway.

I don't think this function really cares about the visibility with
respect to the current snapshot, right? It really cares about what other
transactions are interacting with the tuple and how. And I think HTSV
meets that need a little better.

> > The biggest issue on my mind is what to do about Hot Standby. The
> > authors have a plan, but there is also some resistance to it:
> >
> >
> http://archives.postgresql.org/message-id/23698.1295566621@sss.pgh.pa.us
> >
> > We don't need a perfect solution for 9.1, but it would be nice if
> > we had a viable plan for 9.2.
>
> I don't recall any real opposition to what I sketched out in this
> post, which came after the above-referenced one:
>
> http://archives.postgresql.org/message-id/4D39D5EC0200002500039A19@gw.wicourts.gov
>
> Also, that opposition appears to be based on a misunderstanding of
> the first alternative, which was for sending at most one snapshot per
> commit or rollback of a serializable read write transaction, with
> possible throttling. The alternative needs at most two bits per
> commit or rollback of a serializable read write transaction; although
> I haven't checked whether that can be scared up without adding a
> whole byte. Read only transactions have nothing to do with the
> traffic under either alternative.

Ok, great. When I read that before I thought that WAL might need to be
sent for implicit RO transactions. I will read it more carefully again.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> On Mon, 2011-01-31 at 07:26 -0600, Kevin Grittner wrote:
>>> And why are you reading the infomask directly? Do the existing
>>> visibility functions not suffice?
>>
>> It's possible we re-invented some code somewhere, but I'm not
>> clear on what code from this patch might use what existing
>> function. Could you provide specifics?
>
> In CheckForSerializableConflictOut(), it takes a boolean "valid".

Ah, now I see what you're talking about. Take a look at where that
"valid" flag come from -- the CheckForSerializableConflictOut are
all place right after calls to HeapTupleSatisfiesVisibility. The
"valid" value is what HeapTupleSatisfiesVisibility returned. Is it
possible that the hint bits will not be accurate right after that?
With that in mind, do you still see a problem with how things are
currently done?

-Kevin

On Mon, 2011-01-31 at 13:32 -0600, Kevin Grittner wrote:
> Ah, now I see what you're talking about. Take a look at where that
> "valid" flag come from -- the CheckForSerializableConflictOut are
> all place right after calls to HeapTupleSatisfiesVisibility. The
> "valid" value is what HeapTupleSatisfiesVisibility returned. Is it
> possible that the hint bits will not be accurate right after that?
> With that in mind, do you still see a problem with how things are
> currently done?

Oh, ok. The staleness of the hint bit was a fairly minor point though.

Really, I think this should be using HTSV to separate concerns better
and improve readability. My first reaction was to try to find out what
the function was doing that's special. If it is doing something special,
and HTSV is not what you're really looking for, a comment to explain
would be helpful.

As an example, consider that Robert Haas recently suggested using an
infomask bit to mean frozen, rather than actually removing the xid, to
save the xid as forensic information. If that were to happen, your code
would be reading an xid that may have been re-used.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> I don't think this function really cares about the visibility with
> respect to the current snapshot, right?

What it cares about is whether some other particular top level
transaction wrote a tuple which we *would* read except that it is
not visible to us because that other top level transaction is
concurrent with ours. If so, we want to flag a read-write conflict
out from our transaction and in to that other transaction.

-Kevin

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> Really, I think this should be using HTSV to separate concerns
> better and improve readability. My first reaction was to try to
> find out what the function was doing that's special. If it is
> doing something special, and HTSV is not what you're really
> looking for, a comment to explain would be helpful.

It does seem that at least a comment would be needed. I'm not at
all confident that there isn't some macro or function which would
yield what I need. I just sent an email clarifying exactly what I
want to check, so if you can see a better way to determine that, I'm
all ears.

> As an example, consider that Robert Haas recently suggested using
> an infomask bit to mean frozen, rather than actually removing the
> xid, to save the xid as forensic information. If that were to
> happen, your code would be reading an xid that may have been
> re-used.

Yeah, clearly if the code remains as it is, it would be sensitive to
changes in how hint bits or the xid values are used. If we can
abstract that, it's clearly a Good Thing to do so.

-Kevin

On Mon, 2011-01-31 at 13:55 -0600, Kevin Grittner wrote:
> Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>
> > I don't think this function really cares about the visibility with
> > respect to the current snapshot, right?
>
> What it cares about is whether some other particular top level
> transaction wrote a tuple which we *would* read except that it is
> not visible to us because that other top level transaction is
> concurrent with ours.

Or a tuple that you *are* reading, but is being deleted concurrently,
right? Or has been deleted by an overlapping transaction?

> If so, we want to flag a read-write conflict
> out from our transaction and in to that other transaction.

It still seems like HTSV would suffice, unless I'm missing something.

I think "visible" is still needed though: it matters in the cases
HEAPTUPLE_RECENTLY_DEAD and HEAPTUPLE_LIVE. For the former, it only
allows an early exit (if !visible); but for the latter, I think it's
required.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Mon, 2011-01-31 at 13:55 -0600, Kevin Grittner wrote:

>> What it cares about is whether some other particular top level
>> transaction wrote a tuple which we *would* read except that it is
>> not visible to us because that other top level transaction is
>> concurrent with ours.
>
> Or a tuple that you *are* reading, but is being deleted
> concurrently, right? Or has been deleted by an overlapping
> transaction?

Right. I guess that wasn't as precise a statement as I thought. I
was trying to say that the effects of some write (insert, update,
delete to a permanent table) would not be visible to us because the
writing transaction is concurrent, for some tuple under
consideration.

>> If so, we want to flag a read-write conflict
>> out from our transaction and in to that other transaction.
>
> It still seems like HTSV would suffice, unless I'm missing
> something.

It is at least as likely that I'm missing something. If I'm
following you, we're talking about these 24 lines of code, where
"valid" is the what was just returned from
HeapTupleSatisfiesVisibility:

if (valid)
{
/*
* We may bail out if previous xmax aborted, or if it
committed but
* only locked the tuple without updating it.
*/
if (tuple->t_data->t_infomask & (HEAP_XMAX_INVALID |
HEAP_IS_LOCKED))
return;

/*
* If there's a valid xmax, it must be from a concurrent
transaction,
* since it deleted a tuple which is visible to us.
*/
xid = HeapTupleHeaderGetXmax(tuple->t_data);
if (!TransactionIdIsValid(xid))
return;
}
else
{
/*
* We would read this row, but it isn't visible to us.
*/
xid = HeapTupleHeaderGetXmin(tuple->t_data);
}

We follow this by a check for the top-level xid, and return if
that's early enough to have overlapped our transaction.

This seems to work as intended for a all known tests. I guess my
questions would be:

(1) Do you see a case where this would do the wrong thing? Can you
describe that or (even better) provide a test case to demonstrate
it?

(2) I haven't gotten my head around how HTSV helps or is even the
right thing. If I want to try the switch statement from your recent
post, what should I use as the OldestXmin value on the call to HTSV?

-Kevin

I wrote:

> We follow this by a check for the top-level xid, and return if
> that's early enough to have overlapped our transaction.

s/early enough to have overlapped/early enough not to have
overlapped/

-Kevin

On Mon, 2011-01-31 at 14:38 -0600, Kevin Grittner wrote:
> It is at least as likely that I'm missing something. If I'm
> following you, we're talking about these 24 lines of code, where
> "valid" is the what was just returned from
> HeapTupleSatisfiesVisibility:

Yes.

> (1) Do you see a case where this would do the wrong thing? Can you
> describe that or (even better) provide a test case to demonstrate
> it?

No, I don't see any incorrect results.

> (2) I haven't gotten my head around how HTSV helps or is even the
> right thing.

It primarily just encapsulates the access to the tuple header fields. I
think that avoiding the messy logic of hint bits, tuple locks, etc., is
a significant win for readability and maintainability.

> If I want to try the switch statement from your recent
> post, what should I use as the OldestXmin value on the call to HTSV?

I believe RecentGlobalXmin should work.

And I don't think the original switch statement I posted did the right
thing for HEAPTUPLE_LIVE. I think that case needs to account for the
visible flag (if it's live but not visible, that's the same as
insert-in-progress for your purposes).

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> Ok, great. When I read that before I thought that WAL might need
> to be sent for implicit RO transactions. I will read it more
> carefully again.

In looking back over recent posts to see what I might have missed or
misinterpreted, I now see your point. Either of these alternatives
would involve potentially sending something through the WAL on
commit or rollback of some serializable transactions which *did not*
write anything, if they were not *declared* to be READ ONLY. If
that is not currently happening (again, I confess to not having yet
delved into the mysteries of writing WAL records), then we would
need a new WAL record type for writing these.

That said, the logic would not make it at all useful to send
something for *every* such transaction, and I've rather assumed
that we would want some heuristic for setting a minimum interval
between notifications, whether we sent the snapshots themselves or
just flags to indicate it was time to build or validate a candidate
snapshot.

Sorry for misunderstanding the concerns.

-Kevin

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Mon, 2011-01-31 at 14:38 -0600, Kevin Grittner wrote:

>> If I want to try the switch statement from your recent
>> post, what should I use as the OldestXmin value on the call to
>> HTSV?
>
> I believe RecentGlobalXmin should work.
>
> And I don't think the original switch statement I posted did the
> right thing for HEAPTUPLE_LIVE. I think that case needs to account
> for the visible flag (if it's live but not visible, that's the
> same as insert-in-progress for your purposes).

I'll try to set this up and see if I can get it to pass the check
and dcheck make targets. Can we assume that the performance impact
would be too small to matter when we know for sure that hint bits
have already been set?

-Kevin

On Mon, 2011-01-31 at 15:30 -0600, Kevin Grittner wrote:
> I'll try to set this up and see if I can get it to pass the check
> and dcheck make targets. Can we assume that the performance impact
> would be too small to matter when we know for sure that hint bits
> have already been set?

I think that's a safe assumption. If there is some kind of noticeable
difference in conflict rates or runtime, that probably indicates a bug
in the new or old code.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Mon, 2011-01-31 at 15:30 -0600, Kevin Grittner wrote:
>> I'll try to set this up and see if I can get it to pass the check
>> and dcheck make targets. Can we assume that the performance
>> impact would be too small to matter when we know for sure that
> hint bits have already been set?
>
> I think that's a safe assumption. If there is some kind of
> noticeable difference in conflict rates or runtime, that probably
> indicates a bug in the new or old code.

That worked fine; passed check and dcheck targets.

Here's the code:

http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=6360b0d4ca88c09cf590a75409cd29831afff58b

With confidence that it works, I looked it over some more and now
like this a lot. It is definitely more readable and should be less
fragile in the face of changes to MVCC bit-twiddling techniques. Of
course, any changes to the HTSV_Result enum will require changes to
this code, but that seems easier to spot and fix than the
alternative. Thanks for the suggestion!

Having gotten my head around it, I embellished here:

http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=f9307a41c198a9aa4203eb529f9c6d1b55c5c6e1

Do those changes look reasonable? None of that is really
*necessary*, but it seemed cleaner and clearer that way once I
looked at the code with the changes you suggested.

-Kevin

On Mon, 2011-01-31 at 17:55 -0600, Kevin Grittner wrote:
> http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=6360b0d4ca88c09cf590a75409cd29831afff58b
>
> With confidence that it works, I looked it over some more and now
> like this a lot. It is definitely more readable and should be less
> fragile in the face of changes to MVCC bit-twiddling techniques. Of
> course, any changes to the HTSV_Result enum will require changes to
> this code, but that seems easier to spot and fix than the
> alternative. Thanks for the suggestion!

One thing that confused me a little about the code is the default case
at the end. The enum is exhaustive, so the default doesn't really make
sense. The compiler warning you are silencing is the uninitialized
variable xid (right?), which is clearly a spurious warning. Since you
have the "Assert(TransactionIdIsValid(xid))" there anyway, why not just
initialize xid to InvalidTransactionId and get rid of the default case?
I assume the "Assert(false)" is there to detect if someone adds a new
enum value, but the compiler should issue a warning in that case anyway
(and the comment next to Assert(false) is worded in a confusing way).
This is all really minor stuff, obviously.

Also, from a code standpoint, it might be possible to early return in
the HEAPTUPLE_RECENTLY_DEAD case where visible=false. It looks like it
will be handled quickly afterward (at TransactionIdPrecedes), so you
don't have to change anything, but I thought I would mention it.

> Having gotten my head around it, I embellished here:
>
> http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=f9307a41c198a9aa4203eb529f9c6d1b55c5c6e1
>
> Do those changes look reasonable? None of that is really
> *necessary*, but it seemed cleaner and clearer that way once I
> looked at the code with the changes you suggested.

Yes, I like those changes.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> One thing that confused me a little about the code is the default
> case at the end. The enum is exhaustive, so the default doesn't
> really make sense. The compiler warning you are silencing is the
> uninitialized variable xid (right?)

Right.

> Since you have the "Assert(TransactionIdIsValid(xid))" there
> anyway, why not just initialize xid to InvalidTransactionId and
> get rid of the default case?

I feel a little better keeping even that trivial work out of the
code path if possible, and it seems less confusing to me on the
default case than up front. I'll improve the comment.

> I assume the "Assert(false)" is there to detect if someone adds a
> new enum value, but the compiler should issue a warning in that
> case anyway

My compiler doesn't. Would it make sense to elog here, rather than
Assert? I'm not clear on the rules for that. I'll push something
that way for review and comment. If it's wrong, I'll change it.

> This is all really minor stuff, obviously.

In a million line code base, I hate to call anything which affects
readability minor. ;-)

> Also, from a code standpoint, it might be possible to early return
> in the HEAPTUPLE_RECENTLY_DEAD case where visible=false.

Yeah, good point. It seems worth testing a bool there.

A small push dealing with all the above issues and adding a little
to comments:

http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=538ff57691256de0341e22513f59e9dc4dfd998f

Let me know if any of that still needs work to avoid confusion and
comply with PostgreSQL coding conventions. Like I said, I'm not
totally clear whether elog is right here, but it seems to me a
conceptually similar case to some I found elsewhere that elog was
used.

-Kevin

On Tue, 2011-02-01 at 11:01 -0600, Kevin Grittner wrote:
> My compiler doesn't.

Strange. Maybe it requires -O2?

> Would it make sense to elog here, rather than
> Assert? I'm not clear on the rules for that.

elog looks fine there to me, assuming we have the default case. I'm not
100% clear on the rules, either. I think invalid input/corruption are
usually elog (so they can be caught in non-assert builds); but other
switch statements have them as well ("unrecognized node...").

> A small push dealing with all the above issues and adding a little
> to comments:
>
> http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=538ff57691256de0341e22513f59e9dc4dfd998f
>
> Let me know if any of that still needs work to avoid confusion and
> comply with PostgreSQL coding conventions. Like I said, I'm not
> totally clear whether elog is right here, but it seems to me a
> conceptually similar case to some I found elsewhere that elog was
> used.

Looks good. It also looks like it contains a bugfix for subtransactions,
right?

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Tue, 2011-02-01 at 11:01 -0600, Kevin Grittner wrote:
>> My compiler doesn't.
>
> Strange. Maybe it requires -O2?

That's not it; I see -O2 in my compiles.

At any rate, I think the default clause is the best place to quash
the warning because that leaves us with a warning if changes leave a
path through the other options without setting xid. In other words,
unconditionally setting a value before the switch could prevent
warnings on actual bugs, and I don't see such a risk when it's on
the default.

>> A small push dealing with all the above issues and adding a
>> little to comments:

> Looks good. It also looks like it contains a bugfix for
> subtransactions, right?

I think it fixes a bug introduced in the push from late yesterday.
In reviewing what went into the last push yesterday, it looked like
I might have introduced an assertion failure for the case where
there is a write to a row within a subtransaction and then row is
read again after leaving the subtransaction. I didn't actually
confirm that through testing, because it looked like the safe
approach was better from a performance standpoint, anyway. That
last check for "our own xid" after finding the top level xid comes
before acquiring the LW lock and doing an HTAB lookup which aren't
necessary in that case. Re-reading a row within the same transaction
seems likely enough to make it worth that quick test before doing
more expensive things.

It did throw a scare into me, though. The last thing I want to do
is destabilize things at this juncture. I'll try to be more
conservative with changes from here out.

-Kevin