Re: SSI freezing bug

Hi,

Prompted by Andres Freund's comments on my Freezing without Write I/O
patch, I realized that there's there's an existing bug in the way
predicate locking handles freezing (or rather, it doesn't handle it).

When a tuple is predicate-locked, the key of the lock is ctid+xmin.
However, when a tuple is frozen, its xmin is changed to FrozenXid. That
effectively invalidates any predicate lock on the tuple, as checking for
a lock on the same tuple later won't find it as the xmin is different.

Attached is an isolationtester spec to demonstrate this.

- Heikki

Hi,

On 2013-09-20 13:55:36 +0300, Heikki Linnakangas wrote:
> When a tuple is predicate-locked, the key of the lock is ctid+xmin. However,
> when a tuple is frozen, its xmin is changed to FrozenXid. That effectively
> invalidates any predicate lock on the tuple, as checking for a lock on the
> same tuple later won't find it as the xmin is different.
>
> Attached is an isolationtester spec to demonstrate this.

Do you have any idea to fix that besides keeping the xmin horizon below the
lowest of the xids that are predicate locked? Which seems nasty to
compute and is probably not trivial to fit into the procarray.c
machinery?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-09-20 13:53:04 +0200, Andres Freund wrote:
> Hi,
>
>
> On 2013-09-20 13:55:36 +0300, Heikki Linnakangas wrote:
> > When a tuple is predicate-locked, the key of the lock is ctid+xmin. However,
> > when a tuple is frozen, its xmin is changed to FrozenXid. That effectively
> > invalidates any predicate lock on the tuple, as checking for a lock on the
> > same tuple later won't find it as the xmin is different.
> >
> > Attached is an isolationtester spec to demonstrate this.
>
> Do you have any idea to fix that besides keeping the xmin horizon below the
> lowest of the xids that are predicate locked? Which seems nasty to
> compute and is probably not trivial to fit into the procarray.c
> machinery?

A better solution probably is to promote tuple-level locks if they exist
to a relation level one upon freezing I guess?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> On 2013-09-20 13:55:36 +0300, Heikki Linnakangas wrote:
>>> When a tuple is predicate-locked, the key of the lock is ctid+xmin.
>>> However, when a tuple is frozen, its xmin is changed to FrozenXid. That
>>> effectively
>>> invalidates any predicate lock on the tuple, as checking for a lock on
>>> the
>>> same tuple later won't find it as the xmin is different.
>>>
>>> Attached is an isolationtester spec to demonstrate this.
>>
>> Do you have any idea to fix that besides keeping the xmin horizon below the
>> lowest of the xids that are predicate locked? Which seems nasty to
>> compute and is probably not trivial to fit into the procarray.c
>> machinery?
>
> A better solution probably is to promote tuple-level locks if they exist
> to a relation level one upon freezing I guess?

That would work.  A couple other ideas would be to use the oldest
serializable xmin which is being calculated in predicate.c to
either prevent freezing of newer transaction or to summarize
predicate locks (using the existing SLRU-based summarization
functions) which use xmin values for xids which are being frozen.
The transactions must already be committed, and so are eligible for
summarization.

I'm not sure which is best.  Will review, probably this weekend.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> schrieb:
>Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>>Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>> On 2013-09-20 13:55:36 +0300, Heikki Linnakangas wrote:
>>>>> When a tuple is predicate-locked, the key of the lock is
>ctid+xmin.
>>>>> However, when a tuple is frozen, its xmin is changed to FrozenXid.
>>That
>>>>> effectively
>>>>> invalidates any predicate lock on the tuple, as checking for a
>lock
>>on
>>>>> the
>>>>> same tuple later won't find it as the xmin is different.
>>>>>
>>>>> Attached is an isolationtester spec to demonstrate this.
>>>>
>>>> Do you have any idea to fix that besides keeping the xmin horizon
>>below the
>>>> lowest of the xids that are predicate locked? Which seems nasty to
>>>> compute and is probably not trivial to fit into the procarray.c
>>>> machinery?
>>>
>>> A better solution probably is to promote tuple-level locks if they
>>exist
>>> to a relation level one upon freezing I guess?
>>
>>That would work.  A couple other ideas would be to use the oldest
>>serializable xmin which is being calculated in predicate.c to
>>either prevent freezing of newer transaction or to summarize
>>predicate locks (using the existing SLRU-based summarization
>>functions) which use xmin values for xids which are being frozen.
>>The transactions must already be committed, and so are eligible for
>>summarization.
>
>What's the point of using xmin as part of the lock key in the first
>place, wouldn't ctid alone suffice? If the tuple was visible to the
>reading transaction, it cannot be vacuumed away until the transaction
>commits. No other tuple can appear with the same ctid.

SSI locks can live longer than one TX/snapshot.

Andres

--
Please excuse brevity and formatting - I am writing this on my mobile phone.

Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> On 2013-09-20 13:55:36 +0300, Heikki Linnakangas wrote:
>>>> When a tuple is predicate-locked, the key of the lock is ctid+xmin.
>>>> However, when a tuple is frozen, its xmin is changed to FrozenXid.
>That
>>>> effectively
>>>> invalidates any predicate lock on the tuple, as checking for a lock
>on
>>>> the
>>>> same tuple later won't find it as the xmin is different.
>>>>
>>>> Attached is an isolationtester spec to demonstrate this.
>>>
>>> Do you have any idea to fix that besides keeping the xmin horizon
>below the
>>> lowest of the xids that are predicate locked? Which seems nasty to
>>> compute and is probably not trivial to fit into the procarray.c
>>> machinery?
>>
>> A better solution probably is to promote tuple-level locks if they
>exist
>> to a relation level one upon freezing I guess?
>
>That would work.  A couple other ideas would be to use the oldest
>serializable xmin which is being calculated in predicate.c to
>either prevent freezing of newer transaction or to summarize
>predicate locks (using the existing SLRU-based summarization
>functions) which use xmin values for xids which are being frozen.
>The transactions must already be committed, and so are eligible for
>summarization.

What's the point of using xmin as part of the lock key in the first place, wouldn't ctid alone suffice? If the tuple was visible to the reading transaction, it cannot be vacuumed away until the transaction commits. No other tuple can appear with the same ctid.

- Heikki

On 09/21/2013 10:46 PM, Andres Freund wrote:
>
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> schrieb:
>> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>>> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>>> On 2013-09-20 13:55:36 +0300, Heikki Linnakangas wrote:
>>>>>> When a tuple is predicate-locked, the key of the lock is
>> ctid+xmin.
>>>>>> However, when a tuple is frozen, its xmin is changed to FrozenXid.
>>> That
>>>>>> effectively
>>>>>> invalidates any predicate lock on the tuple, as checking for a
>> lock
>>> on
>>>>>> the
>>>>>> same tuple later won't find it as the xmin is different.
>>>>>>
>>>>>> Attached is an isolationtester spec to demonstrate this.
>>>>> Do you have any idea to fix that besides keeping the xmin horizon
>>> below the
>>>>> lowest of the xids that are predicate locked? Which seems nasty to
>>>>> compute and is probably not trivial to fit into the procarray.c
>>>>> machinery?
>>>> A better solution probably is to promote tuple-level locks if they
>>> exist
>>>> to a relation level one upon freezing I guess?
>>> That would work. A couple other ideas would be to use the oldest
>>> serializable xmin which is being calculated in predicate.c to
>>> either prevent freezing of newer transaction or to summarize
>>> predicate locks (using the existing SLRU-based summarization
>>> functions) which use xmin values for xids which are being frozen.
>>> The transactions must already be committed, and so are eligible for
>>> summarization.
>> What's the point of using xmin as part of the lock key in the first
>> place, wouldn't ctid alone suffice? If the tuple was visible to the
>> reading transaction, it cannot be vacuumed away until the transaction
>> commits. No other tuple can appear with the same ctid.
> SSI locks can live longer than one TX/snapshot.
But the only way that ctid can change without xmin changing is when
you update the tuple in the same TX that created it.

Could it be the case here or can we safely exclude this ?

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 09/20/2013 12:55 PM, Heikki Linnakangas wrote:
> Hi,
>
> Prompted by Andres Freund's comments on my Freezing without Write I/O
> patch, I realized that there's there's an existing bug in the way
> predicate locking handles freezing (or rather, it doesn't handle it).
>
> When a tuple is predicate-locked, the key of the lock is ctid+xmin.
> However, when a tuple is frozen, its xmin is changed to FrozenXid.
> That effectively invalidates any predicate lock on the tuple, as
> checking for a lock on the same tuple later won't find it as the xmin
> is different.
>
> Attached is an isolationtester spec to demonstrate this.
The case is even fishier than that.

That is, you can get bad behaviour on at least v9.2.4 even without
VACUUM FREEZE.

You just need to run

permutation "r1" "r2" "w1" "w2" "c1" "c2"

twice in a row.

the first time it does get serialization error at "c2"
but the 2nd time both "c1" and "c2" complete successfully

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 22.09.2013 00:12, Hannu Krosing wrote:
> On 09/21/2013 10:46 PM, Andres Freund wrote:
>>
>> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> schrieb:
>>> Kevin Grittner<kgrittn(at)ymail(dot)com> wrote:
>>>> Andres Freund<andres(at)2ndquadrant(dot)com> wrote:
>>>>>> On 2013-09-20 13:55:36 +0300, Heikki Linnakangas wrote:
>>>>>>> When a tuple is predicate-locked, the key of the lock is
>>> ctid+xmin.
>>>>>>> However, when a tuple is frozen, its xmin is changed to FrozenXid.
>>>> That
>>>>>>> effectively
>>>>>>> invalidates any predicate lock on the tuple, as checking for a
>>> lock
>>>> on
>>>>>>> the
>>>>>>> same tuple later won't find it as the xmin is different.
>>>>>>>
>>>>>>> Attached is an isolationtester spec to demonstrate this.
>>>>>> Do you have any idea to fix that besides keeping the xmin horizon
>>>> below the
>>>>>> lowest of the xids that are predicate locked? Which seems nasty to
>>>>>> compute and is probably not trivial to fit into the procarray.c
>>>>>> machinery?
>>>>> A better solution probably is to promote tuple-level locks if they
>>>> exist
>>>>> to a relation level one upon freezing I guess?
>>>> That would work. A couple other ideas would be to use the oldest
>>>> serializable xmin which is being calculated in predicate.c to
>>>> either prevent freezing of newer transaction or to summarize
>>>> predicate locks (using the existing SLRU-based summarization
>>>> functions) which use xmin values for xids which are being frozen.
>>>> The transactions must already be committed, and so are eligible for
>>>> summarization.
>>> What's the point of using xmin as part of the lock key in the first
>>> place, wouldn't ctid alone suffice? If the tuple was visible to the
>>> reading transaction, it cannot be vacuumed away until the transaction
>>> commits. No other tuple can appear with the same ctid.
>> SSI locks can live longer than one TX/snapshot.
> But the only way that ctid can change without xmin changing is when
> you update the tuple in the same TX that created it.

I don't think that's relevant. We're not talking about the "ctid" field
of a tuple, ie. HeapTupleHeader.t_ctid. We're talking about its physical
location, ie. HeapTuple->t_self.

- Heikki

On 21.09.2013 23:46, Andres Freund wrote:
>
>
> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> schrieb:
>> Kevin Grittner<kgrittn(at)ymail(dot)com> wrote:
>>> Andres Freund<andres(at)2ndquadrant(dot)com> wrote:
>>>>> On 2013-09-20 13:55:36 +0300, Heikki Linnakangas wrote:
>>>>>> When a tuple is predicate-locked, the key of the lock is
>> ctid+xmin.
>>>>>> However, when a tuple is frozen, its xmin is changed to FrozenXid.
>>> That
>>>>>> effectively
>>>>>> invalidates any predicate lock on the tuple, as checking for a
>> lock
>>> on
>>>>>> the
>>>>>> same tuple later won't find it as the xmin is different.
>>>>>>
>>>>>> Attached is an isolationtester spec to demonstrate this.
>>>>>
>>>>> Do you have any idea to fix that besides keeping the xmin horizon
>>> below the
>>>>> lowest of the xids that are predicate locked? Which seems nasty to
>>>>> compute and is probably not trivial to fit into the procarray.c
>>>>> machinery?
>>>>
>>>> A better solution probably is to promote tuple-level locks if they
>>> exist
>>>> to a relation level one upon freezing I guess?
>>>
>>> That would work. A couple other ideas would be to use the oldest
>>> serializable xmin which is being calculated in predicate.c to
>>> either prevent freezing of newer transaction or to summarize
>>> predicate locks (using the existing SLRU-based summarization
>>> functions) which use xmin values for xids which are being frozen.
>>> The transactions must already be committed, and so are eligible for
>>> summarization.
>>
>> What's the point of using xmin as part of the lock key in the first
>> place, wouldn't ctid alone suffice? If the tuple was visible to the
>> reading transaction, it cannot be vacuumed away until the transaction
>> commits. No other tuple can appear with the same ctid.
>
> SSI locks can live longer than one TX/snapshot.

Hmm, true.

Another idea: we could vacuum away predicate locks, when the
corresponding tuples are vacuumed from the heap. Before the 2nd phase of
vacuum, before removing the dead tuples, we could scan the predicate
lock hash and remove any locks belonging to the tuples we're about to
remove. And not include xmin in the lock key.

- Heikki

On 23.09.2013 01:07, Hannu Krosing wrote:
> On 09/20/2013 12:55 PM, Heikki Linnakangas wrote:
>> Hi,
>>
>> Prompted by Andres Freund's comments on my Freezing without Write I/O
>> patch, I realized that there's there's an existing bug in the way
>> predicate locking handles freezing (or rather, it doesn't handle it).
>>
>> When a tuple is predicate-locked, the key of the lock is ctid+xmin.
>> However, when a tuple is frozen, its xmin is changed to FrozenXid.
>> That effectively invalidates any predicate lock on the tuple, as
>> checking for a lock on the same tuple later won't find it as the xmin
>> is different.
>>
>> Attached is an isolationtester spec to demonstrate this.
> The case is even fishier than that.
>
> That is, you can get bad behaviour on at least v9.2.4 even without
> VACUUM FREEZE.
>
> You just need to run
>
> permutation "r1" "r2" "w1" "w2" "c1" "c2"
>
> twice in a row.
>
> the first time it does get serialization error at "c2"
> but the 2nd time both "c1" and "c2" complete successfully

Oh, interesting. I did some debugging on this: there are actually *two*
bugs, either one of which alone is enough to cause this on its own:

1. in heap_hot_search_buffer(), the PredicateLockTuple() call is passed
wrong offset number. heapTuple->t_self is set to the tid of the first
tuple in the chain that's visited, not the one actually being read.

2. CheckForSerializableConflictIn() uses the tuple's t_ctid field
instead of t_self to check for exiting predicate locks on the tuple. If
the tuple was updated, but the updater rolled back, t_ctid points to the
aborted dead tuple.

After fixing both of those bugs, running the test case twice in a row
works, ie. causes a conflict and a rollback both times. Anyone see a
problem with this?

That still leaves the original problem I spotted, with freezing; that's
yet another unrelated bug.

- Heikki

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> A better solution probably is to promote tuple-level locks if
> they exist to a relation level one upon freezing I guess?

It would be sufficient to promote the tuple lock to a page lock.
It would be pretty easy to add a function to predicate.c which
would accept a Relation and HeapTuple, check for a predicate lock
for the tuple, and add a page lock if found (which will
automatically clear the tuple lock).  This new function would be
called when a tuple was chosen for freezing.  Since freezing always
causes WAL-logging and disk I/O, the cost of a couple hash table
operations should not be noticeable.

This seems like a bug fix which should be back-patched to 9.1, yes?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-10-01 07:41:46 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> > A better solution probably is to promote tuple-level locks if
> > they exist to a relation level one upon freezing I guess?
>
> It would be sufficient to promote the tuple lock to a page lock.
> It would be pretty easy to add a function to predicate.c which
> would accept a Relation and HeapTuple, check for a predicate lock
> for the tuple, and add a page lock if found (which will
> automatically clear the tuple lock).  This new function would be
> called when a tuple was chosen for freezing.  Since freezing always
> causes WAL-logging and disk I/O, the cost of a couple hash table
> operations should not be noticeable.

Yea, not sure why I was thinking of table level locks.

> This seems like a bug fix which should be back-patched to 9.1, yes?

Yes.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-10-01 07:41:46 -0700, Kevin Grittner wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>
>>> A better solution probably is to promote tuple-level locks if
>>> they exist to a relation level one upon freezing I guess?
>>
>> It would be sufficient to promote the tuple lock to a page lock.
>> It would be pretty easy to add a function to predicate.c which
>> would accept a Relation and HeapTuple, check for a predicate lock
>> for the tuple, and add a page lock if found (which will
>> automatically clear the tuple lock).  This new function would be
>> called when a tuple was chosen for freezing.  Since freezing always
>> causes WAL-logging and disk I/O, the cost of a couple hash table
>> operations should not be noticeable.
>
> Yea, not sure why I was thinking of table level locks.
>
>> This seems like a bug fix which should be back-patched to 9.1, yes?
>
> Yes.

Patch attached, including new isolation test based on Heikki's
example.  This patch does change the signature of
heap_freeze_tuple().  If anyone thinks there is risk that external
code may be calling this, I could keep the old function with its
old behavior (including the bug) and add a new function name with
the new parameters added -- the old function could call the new one
with NULL for the last two parameters.  I'm not sure whether that
is better than breaking a compile of code which uses the old
signature, which would force a choice about what to do.

Will give it a couple days for feedback before pushing.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 03.10.2013 01:05, Kevin Grittner wrote:
> Andres Freund<andres(at)2ndquadrant(dot)com> wrote:
>> On 2013-10-01 07:41:46 -0700, Kevin Grittner wrote:
>>> Andres Freund<andres(at)2ndquadrant(dot)com> wrote:
>>>
>>>> A better solution probably is to promote tuple-level locks if
>>>> they exist to a relation level one upon freezing I guess?
>>>
>>> It would be sufficient to promote the tuple lock to a page lock.
>>> It would be pretty easy to add a function to predicate.c which
>>> would accept a Relation and HeapTuple, check for a predicate lock
>>> for the tuple, and add a page lock if found (which will
>>> automatically clear the tuple lock). This new function would be
>>> called when a tuple was chosen for freezing. Since freezing always
>>> causes WAL-logging and disk I/O, the cost of a couple hash table
>>> operations should not be noticeable.
>>
>> Yea, not sure why I was thinking of table level locks.
>>
>>> This seems like a bug fix which should be back-patched to 9.1, yes?
>>
>> Yes.
>
> Patch attached, including new isolation test based on Heikki's
> example. This patch does change the signature of
> heap_freeze_tuple(). If anyone thinks there is risk that external
> code may be calling this, I could keep the old function with its
> old behavior (including the bug) and add a new function name with
> the new parameters added -- the old function could call the new one
> with NULL for the last two parameters. I'm not sure whether that
> is better than breaking a compile of code which uses the old
> signature, which would force a choice about what to do.
>
> Will give it a couple days for feedback before pushing.

IMHO it would be better to remove xmin from the lock key, and vacuum
away the old predicate locks when the corresponding tuple is vacuumed.
The xmin field is only required to handle the case that a tuple is
vacuumed, and a new unrelated tuple is inserted to the same slot.
Removing the lock when the tuple is removed fixes that.

In fact, I cannot even come up with a situation where you would have a
problem if we just removed xmin from the key, even if we didn't vacuum
away old locks. I don't think the old lock can conflict with anything
that would see the new tuple that gets inserted in the same slot. I have
a feeling that you could probably prove that if you stare long enough at
the diagram of a dangerous structure and the properties required for a
conflict.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:

> IMHO it would be better to remove xmin from the lock key, and vacuum
> away the old predicate locks when the corresponding tuple is vacuumed.
> The xmin field is only required to handle the case that a tuple is
> vacuumed, and a new unrelated tuple is inserted to the same slot.
> Removing the lock when the tuple is removed fixes that.
>
> In fact, I cannot even come up with a situation where you would have a
> problem if we just removed xmin from the key, even if we didn't vacuum
> away old locks. I don't think the old lock can conflict with anything
> that would see the new tuple that gets inserted in the same slot. I have
> a feeling that you could probably prove that if you stare long enough at
> the diagram of a dangerous structure and the properties required for a
> conflict.

You are the one who suggested adding xmin to the key:

http://www.postgresql.org/message-id/4D5A36FC.6010203@enterprisedb.com

I will review that thread in light of your recent comments, but the
fact is that xmin was not originally in the lock key, testing
uncovered bugs, and adding xmin fixed those bugs.  I know I tried
some other approach first, which turned out to be complex and quite
messy -- it may have been similar to what you are proposing now.

It seems to me that a change such as you are now suggesting is
likely to be too invasive to back-patch.  Do you agree that it
would make sense to apply the patch I have proposed, back to 9.1,
and then consider any alternative as 9.4 material?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Oct 03, 2013 at 06:19:49AM -0700, Kevin Grittner wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
> > IMHO it would be better to remove xmin from the lock key, and vacuum
> > away the old predicate locks when the corresponding tuple is vacuumed.
> > The xmin field is only required to handle the case that a tuple is
> > vacuumed, and a new unrelated tuple is inserted to the same slot.
> > Removing the lock when the tuple is removed fixes that.

This seems definitely safe: we need the predicate locks to determine if
someone is modifying a tuple we read, and certainly if it's eligible
for vacuum nobody's going to be modifying that tuple anymore.

> > In fact, I cannot even come up with a situation where you would have a
> > problem if we just removed xmin from the key, even if we didn't vacuum
> > away old locks. I don't think the old lock can conflict with anything
> > that would see the new tuple that gets inserted in the same slot. I have
> > a feeling that you could probably prove that if you stare long enough at
> > the diagram of a dangerous structure and the properties required for a
> > conflict.

This would also be safe, in the sense that it's OK to flag a
conflict even if one doesn't exist. I'm not convinced that it isn't
possible to have false positives this way. I think it's possible for a
tuple to be vacuumed away and the ctid reused before the predicate
locks on it are eligible for cleanup. (In fact, isn't this what was
happening in the thread Kevin linked?)

> You are the one who suggested adding xmin to the key:
>
> http://www.postgresql.org/message-id/4D5A36FC.6010203@enterprisedb.com
>
> I will review that thread in light of your recent comments, but the
> fact is that xmin was not originally in the lock key, testing
> uncovered bugs, and adding xmin fixed those bugs.  I know I tried
> some other approach first, which turned out to be complex and quite
> messy -- it may have been similar to what you are proposing now.

At the time, we thought it was necessary for a predicate lock to lock
*all future versions* of a tuple, and so we had a bunch of code to
maintain a version chain. That was fraught with bugs, and turned out
not to be necessary (IIRC, we worked that out at the pub at PGcon).
That made it critical to distinguish different tuples that had the same
ctid because they could wind up in the wrong chain or cause a cycle.
With that code ripped out, that's no longer an issue.

But all this is an exceptionally subtle part of what was an
exceptionally complex patch, so a lot of careful thought is needed
here...

> It seems to me that a change such as you are now suggesting is
> likely to be too invasive to back-patch.  Do you agree that it
> would make sense to apply the patch I have proposed, back to 9.1,
> and then consider any alternative as 9.4 material?

I agree with this.

Dan

--
Dan R. K. Ports UW CSE http://drkp.net/

On 2013-10-03 21:14:17 -0700, Dan Ports wrote:
> On Thu, Oct 03, 2013 at 06:19:49AM -0700, Kevin Grittner wrote:
> > Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
> > > IMHO it would be better to remove xmin from the lock key, and vacuum
> > > away the old predicate locks when the corresponding tuple is vacuumed.
> > > The xmin field is only required to handle the case that a tuple is
> > > vacuumed, and a new unrelated tuple is inserted to the same slot.
> > > Removing the lock when the tuple is removed fixes that.
>
> This seems definitely safe: we need the predicate locks to determine if
> someone is modifying a tuple we read, and certainly if it's eligible
> for vacuum nobody's going to be modifying that tuple anymore.

But we're talking about freezing a tuple, not removing a dead tuple. I
don't see anything preventing modification of a frozen tuple. Right?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04.10.2013 13:23, Andres Freund wrote:
> On 2013-10-03 21:14:17 -0700, Dan Ports wrote:
>> On Thu, Oct 03, 2013 at 06:19:49AM -0700, Kevin Grittner wrote:
>>> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> wrote:
>>>> IMHO it would be better to remove xmin from the lock key, and vacuum
>>>> away the old predicate locks when the corresponding tuple is vacuumed.
>>>> The xmin field is only required to handle the case that a tuple is
>>>> vacuumed, and a new unrelated tuple is inserted to the same slot.
>>>> Removing the lock when the tuple is removed fixes that.
>>
>> This seems definitely safe: we need the predicate locks to determine if
>> someone is modifying a tuple we read, and certainly if it's eligible
>> for vacuum nobody's going to be modifying that tuple anymore.
>
> But we're talking about freezing a tuple, not removing a dead tuple. I
> don't see anything preventing modification of a frozen tuple. Right?

Right, but if we no longer include xmin in the lock key, freezing a
tuple makes no difference. Currently, the problem is that when a tuple
is frozen, the locks on the tuple on the tuple are "lost", as the
xmin+ctid of the lock no longer matches xmin+ctid of the tuple.

Removing xmin from the lock altogether solves that problem, but it
introduces the possibility that when an old tuple is vacuumed away and a
new tuple is inserted on the same slot, a lock on the old tuple is
confused to be a lock on the new tuple. And that problem can be fixed by
vacuuming locks, when the corresponding tuple is vacuumed.

- Heikki

On 2013-10-04 13:51:00 +0300, Heikki Linnakangas wrote:
> On 04.10.2013 13:23, Andres Freund wrote:
> >On 2013-10-03 21:14:17 -0700, Dan Ports wrote:
> >>On Thu, Oct 03, 2013 at 06:19:49AM -0700, Kevin Grittner wrote:
> >>>Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> wrote:
> >>>>IMHO it would be better to remove xmin from the lock key, and vacuum
> >>>>away the old predicate locks when the corresponding tuple is vacuumed.
> >>>>The xmin field is only required to handle the case that a tuple is
> >>>>vacuumed, and a new unrelated tuple is inserted to the same slot.
> >>>>Removing the lock when the tuple is removed fixes that.
> >>
> >>This seems definitely safe: we need the predicate locks to determine if
> >>someone is modifying a tuple we read, and certainly if it's eligible
> >>for vacuum nobody's going to be modifying that tuple anymore.
> >
> >But we're talking about freezing a tuple, not removing a dead tuple. I
> >don't see anything preventing modification of a frozen tuple. Right?
>
> Right, but if we no longer include xmin in the lock key, freezing a tuple
> makes no difference. Currently, the problem is that when a tuple is frozen,
> the locks on the tuple on the tuple are "lost", as the xmin+ctid of the lock
> no longer matches xmin+ctid of the tuple.
>
> Removing xmin from the lock altogether solves that problem, but it
> introduces the possibility that when an old tuple is vacuumed away and a new
> tuple is inserted on the same slot, a lock on the old tuple is confused to
> be a lock on the new tuple. And that problem can be fixed by vacuuming
> locks, when the corresponding tuple is vacuumed.

But locks on those still can have meaning, right? From my very limited
understanding of predicate.c/SSI I don't see why we cannot have
meaningful conflicts on tuples that are already vacuumable. You'd need
some curiously interleaved transactions, sure, but it seems possible?

ISTM we'd need to peg the xmin horizon for vacuum to the oldest xmin
predicate.c keeps track of.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04.10.2013 14:02, Andres Freund wrote:
> But locks on those still can have meaning, right? From my very limited
> understanding of predicate.c/SSI I don't see why we cannot have
> meaningful conflicts on tuples that are already vacuumable. You'd need
> some curiously interleaved transactions, sure, but it seems possible?

To conflict with a lock, a backend would need to read or update the
tuple the lock is on. If the tuple is vacuumable, it's no longer visible
to anyone, so no backend can possibly read or update it anymore.

- Heikki

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> On 2013-10-03 21:14:17 -0700, Dan Ports wrote:
>>>>> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com>  wrote:
>>>>>> IMHO it would be better to remove xmin from the lock key,
>>>>>> and vacuum away the old predicate locks when the
>>>>>> corresponding tuple is vacuumed.
>>>>>> The xmin field is only required to handle the case that a
>>>>>> tuple is vacuumed, and a new unrelated tuple is inserted to
>>>>>> the same slot.
>>>>>> Removing the lock when the tuple is removed fixes that.
>>>>
>>>> This seems definitely safe: we need the predicate locks to
>>>> determine if someone is modifying a tuple we read, and
>>>> certainly if it's eligible for vacuum nobody's going to be
>>>> modifying that tuple anymore.

I totally agree.  It would be safe, and generally seems better, to
omit xmin from the hash tag for heap tuple locks.

> But locks on those still can have meaning, right? From my very
> limited understanding of predicate.c/SSI I don't see why we
> cannot have meaningful conflicts on tuples that are already
> vacuumable. You'd need some curiously interleaved transactions,
> sure, but it seems possible?

No.  We established quite firmly that predicate locks on a tuple do
not need to be carried forward to new versions of that tuple.  It
is only the *version* of the row that was read to create the
predicate lock which needs to be monitored for write conflicts.  If
the row has been vacuumed, or even determined to be eligible for
vacuuming, we know it is not a candidate for update or delete -- so
it is safe to drop the predicate locks.  We do *not* want to do any
other cleanup for the transaction which read that tuple based on
this; just the individual tuple lock should be cleared.  Any
conflict with the lock which occurred earlier will be preserved.

> ISTM we'd need to peg the xmin horizon for vacuum to the oldest
> xmin predicate.c keeps track of.

That would be another alternative, but it seems more problematic
and less effective.

I'm strongly leaning toward the idea that a slightly tweaked
version of the proposed patch is appropriate for the back-branches,
because the fix Heikki is now suggesting seems too invasive to
back-patch.  I think it would make sense to apply it to master,
too, so that the new isolation tests can be immediately added.  We
can then work on the new approach for 9.4 and have the tests to
help confirm we are not breaking anything.  The tweak would be to
preserve the signature of heap_freeze_tuple(), because after the
more invasive fix in 9.4 the new parameters will not be needed.
They are only passed as non-NULL from one of the three callers, so
it seems best to leave those call sites alone rather than change
them back-and-forth.

I will post a new patch today or tomorrow.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

> I'm strongly leaning toward the idea that a slightly tweaked
> version of the proposed patch is appropriate for the back-branches,
> because the fix Heikki is now suggesting seems too invasive to
> back-patch.  I think it would make sense to apply it to master,
> too, so that the new isolation tests can be immediately added.  We
> can then work on the new approach for 9.4 and have the tests to
> help confirm we are not breaking anything.  The tweak would be to
> preserve the signature of heap_freeze_tuple(), because after the
> more invasive fix in 9.4 the new parameters will not be needed.
> They are only passed as non-NULL from one of the three callers, so
> it seems best to leave those call sites alone rather than change
> them back-and-forth.
>
> I will post a new patch today or tomorrow.

Attached.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
>
>> I'm strongly leaning toward the idea that a slightly tweaked
>> version of the proposed patch is appropriate for the back-branches,
>> because the fix Heikki is now suggesting seems too invasive to
>> back-patch.  I think it would make sense to apply it to master,
>> too, so that the new isolation tests can be immediately added.  We
>> can then work on the new approach for 9.4 and have the tests to
>> help confirm we are not breaking anything.  The tweak would be to
>> preserve the signature of heap_freeze_tuple(), because after the
>> more invasive fix in 9.4 the new parameters will not be needed.
>> They are only passed as non-NULL from one of the three callers, so
>> it seems best to leave those call sites alone rather than change
>> them back-and-forth.
>>
>> I will post a new patch today or tomorrow.
>
> Attached.

And here's a rough cut of what I think the alternative now
suggested by Heikki would look like.  (I've omitted the new
isolation test because that is the same as the previously posted
patch.)

Note this comment, which I think was written by Heikki back when
there was a lot more benchmarking and profiling of SSI going on:

  * Because a particular target might become obsolete, due to update to a new
  * version, before the reading transaction is obsolete, we need some way to
  * prevent errors from reuse of a tuple ID.  Rather than attempting to clean
  * up the targets as the related tuples are pruned or vacuumed, we check the
  * xmin on access.    This should be far less costly.

Based on what this patch looks like, I'm afraid he may have been
right when he wrote that.  In any event, after the exercise of
developing a first draft of searching for predicate locks to clean
up every time a tuple is pruned or vacuumed, I continue to feel
strongly that the previously-posted patch, which only takes action
when tuples are frozen by a vacuum process, is much more suitable
for backpatching.  I don't think we should switch to anything
resembling the attached without some careful benchmarking.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06.10.2013 20:34, Kevin Grittner wrote:
> Note this comment, which I think was written by Heikki back when
> there was a lot more benchmarking and profiling of SSI going on:
>
> * Because a particular target might become obsolete, due to update to a new
> * version, before the reading transaction is obsolete, we need some way to
> * prevent errors from reuse of a tuple ID. Rather than attempting to clean
> * up the targets as the related tuples are pruned or vacuumed, we check the
> * xmin on access. This should be far less costly.
>
> Based on what this patch looks like, I'm afraid he may have been
> right when he wrote that. In any event, after the exercise of
> developing a first draft of searching for predicate locks to clean
> up every time a tuple is pruned or vacuumed, I continue to feel
> strongly that the previously-posted patch, which only takes action
> when tuples are frozen by a vacuum process, is much more suitable
> for backpatching. I don't think we should switch to anything
> resembling the attached without some careful benchmarking.

Hmm, you're probably right. I was thinking that the overhead of scanning
the lock hash on a regular vacuum is negligible, but I didn't consider
pruning. It might be significant there.

I'd like to give this line of investigation some more thought:

> On 04.10.2013 07:14, Dan Ports wrote:
>> On Thu, Oct 03, 2013 at 06:19:49AM -0700, Kevin Grittner wrote:
>>> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> wrote:
>>>> IMHO it would be better to remove xmin from the lock key, and vacuum
>>>> away the old predicate locks when the corresponding tuple is vacuumed.
>>>> The xmin field is only required to handle the case that a tuple is
>>>> vacuumed, and a new unrelated tuple is inserted to the same slot.
>>>> Removing the lock when the tuple is removed fixes that.
>>
>> This seems definitely safe: we need the predicate locks to determine if
>> someone is modifying a tuple we read, and certainly if it's eligible
>> for vacuum nobody's going to be modifying that tuple anymore.
>>
>>>> In fact, I cannot even come up with a situation where you would have a
>>>> problem if we just removed xmin from the key, even if we didn't vacuum
>>>> away old locks. I don't think the old lock can conflict with anything
>>>> that would see the new tuple that gets inserted in the same slot. I have
>>>> a feeling that you could probably prove that if you stare long enough at
>>>> the diagram of a dangerous structure and the properties required for a
>>>> conflict.
>>
>> This would also be safe, in the sense that it's OK to flag a
>> conflict even if one doesn't exist. I'm not convinced that it isn't
>> possible to have false positives this way. I think it's possible for a
>> tuple to be vacuumed away and the ctid reused before the predicate
>> locks on it are eligible for cleanup. (In fact, isn't this what was
>> happening in the thread Kevin linked?)

Time to stare at the dangerous structure again:

> SSI is based on the observation [2] that each snapshot isolation
> anomaly corresponds to a cycle that contains a "dangerous structure"
> of two adjacent rw-conflict edges:
>
> Tin ------> Tpivot ------> Tout
> rw rw
>

Furthermore, Tout must commit first.

The following are true:

* A transaction can only hold a predicate lock on a tuple that's visible
to it.

* A tuple that's visible to Tin or Tpivot cannot be vacuumed away until
Tout commits.

When updating a tuple, CheckTargetForConflictsIn() only marks a conflict
if the transaction holding the predicate lock overlapped with the
updating transaction. And if a tuple is vacuumed away and the slot is
reused, an transaction updating the new tuple cannot overlap with any
transaction holding a lock on the old tuple. Otherwise the tuple
wouldn't have been eligible for vacuuming in the first place.

So I don't think you can ever get a false conflict because of slot
reuse. And if there's a hole in that thinking I can't see right now, the
worst that will happen is some unnecessary conflicts, ie. it's still
correct. It surely can't be worse than upgrading the lock to a
page-level lock, which would also create unnecessary conflicts.

Summary: IMHO we should just remove xmin from the predicate lock tag.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:

> So I don't think you can ever get a false conflict because of
> slot reuse.

I spent some time looking at this, and I now agree.

> And if there's a hole in that thinking I can't see right now, the
> worst that will happen is some unnecessary conflicts, ie. it's
> still correct.

That is definitely true; no doubt about that part.

> Summary: IMHO we should just remove xmin from the predicate lock
> tag.

I spent some time trying to see how the vacuum could happen at a
point that could cause a false positive, and was unable to do so.
Even if that is just a failure of imagination on my part, the above
argument that it doesn't produce incorrect results still holds.  I
think the fact that I couldn't find a sequence of events which
would trigger a false positive suggests it would be a rare case,
anyway.

I found the original bug report which led to the addition of xmin
to the tag, and it was because we were still carrying tuple locks
forward to new versions of those locks at the time.  This was later
proven to be unnecessary, which simplified other code; we
apparently missed a trick in not removing xmin from the lock tag at
that point.  Since leaving it there has now been shown to *cause* a
bug, I'm inclined to agree that we should simply remove it, and
backpatch that.

Patch attached.  Any objections to applying that Real Soon Now?
(When, exactly is the deadline to make today's minor release
cut-off?)

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

> Patch attached.  Any objections to applying that Real Soon Now?

Oh, without the new line in predicate.h.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote:
> Patch attached.  Any objections to applying that Real Soon Now?
> (When, exactly is the deadline to make today's minor release
> cut-off?)

Maybe it's overly careful, but I personally slightly vote for applying
it after the backbranch releases. The current behaviour doesn't have any
harsh consequences and mostly reproduceable in artifical scenarios and
the logic here is complex enough that we might miss something.

A day just doesn't leave much time to noticing any issues.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 07.10.2013 16:58, Andres Freund wrote:
> On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote:
>> Patch attached. Any objections to applying that Real Soon Now?
>> (When, exactly is the deadline to make today's minor release
>> cut-off?)
>
> Maybe it's overly careful, but I personally slightly vote for applying
> it after the backbranch releases. The current behaviour doesn't have any
> harsh consequences and mostly reproduceable in artifical scenarios and
> the logic here is complex enough that we might miss something.

Well, it's fairly harsh if the feature isn't working as advertised.

> A day just doesn't leave much time to noticing any issues.

It's less than ideal, I agree, but it doesn't seem better to me to just
leave the bug unfixed for another minor release. Even if we sit on this
for another few months, I don't think we'll get any meaningful amount of
extra testing on it.

- Heikki

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote:
>
>> Patch attached.  Any objections to applying that Real Soon Now?
>> (When, exactly is the deadline to make today's minor release
>> cut-off?)
>
> Maybe it's overly careful, but I personally slightly vote for applying
> it after the backbranch releases. The current behaviour doesn't have any
> harsh consequences and mostly reproduceable in artifical scenarios and
> the logic here is complex enough that we might miss something.
>
> A day just doesn't leave much time to noticing any issues.

I grant that the bug in existing production code is not likely to
get hit very often, but it is a bug; the new isolation test shows
the bug clearly and shows that the suggested patch fixes it.  What
tips the scales for me is that the only possible downside if we
missed something is an occasional false positive serialization
failure, which does not break correctness -- we try to minimize
those for performance reasons, but the algorithm allows them and
they currently do happen.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 07.10.2013 16:44, Kevin Grittner wrote:
> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> wrote:
>
>> So I don't think you can ever get a false conflict because of
>> slot reuse.
>
> I spent some time looking at this, and I now agree.
>
>> And if there's a hole in that thinking I can't see right now, the
>> worst that will happen is some unnecessary conflicts, ie. it's
>> still correct.
>
> That is definitely true; no doubt about that part.
>
>> Summary: IMHO we should just remove xmin from the predicate lock
>> tag.
>
> I spent some time trying to see how the vacuum could happen at a
> point that could cause a false positive, and was unable to do so.
> Even if that is just a failure of imagination on my part, the above
> argument that it doesn't produce incorrect results still holds. I
> think the fact that I couldn't find a sequence of events which
> would trigger a false positive suggests it would be a rare case,
> anyway.
>
> I found the original bug report which led to the addition of xmin
> to the tag, and it was because we were still carrying tuple locks
> forward to new versions of those locks at the time. This was later
> proven to be unnecessary, which simplified other code; we
> apparently missed a trick in not removing xmin from the lock tag at
> that point. Since leaving it there has now been shown to *cause* a
> bug, I'm inclined to agree that we should simply remove it, and
> backpatch that.
>
> Patch attached. Any objections to applying that Real Soon Now?
> (When, exactly is the deadline to make today's minor release
> cut-off?)

A comment somewhere would be nice to explain why we're no longer worried
about confusing an old tuple version with a new tuple in the same slot.
Not sure where.

- Heikki

On 2013-10-07 17:07:16 +0300, Heikki Linnakangas wrote:
> On 07.10.2013 16:58, Andres Freund wrote:
> >On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote:
> >>Patch attached. Any objections to applying that Real Soon Now?
> >>(When, exactly is the deadline to make today's minor release
> >>cut-off?)
> >
> >Maybe it's overly careful, but I personally slightly vote for applying
> >it after the backbranch releases. The current behaviour doesn't have any
> >harsh consequences and mostly reproduceable in artifical scenarios and
> >the logic here is complex enough that we might miss something.
>
> Well, it's fairly harsh if the feature isn't working as advertised.

Well, you need to have a predicate lock on a tuple that's getting frozen
(i.e. hasn't been written to for 50mio+ xids) and you need to have an
update during the time that predicate lock is held. That's not too
likely without explicit VACUUM FREEZEs interleaved there somewhere.

> >A day just doesn't leave much time to noticing any issues.
>
> It's less than ideal, I agree, but it doesn't seem better to me to just
> leave the bug unfixed for another minor release. Even if we sit on this for
> another few months, I don't think we'll get any meaningful amount of extra
> testing on it.

Yea, maybe. Although HEAD does get some testing...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:

> Well, it's fairly harsh if the feature isn't working as
> advertised.

Right -- there are people counting on serializable transaction to
avoid data corruption (creation of data which violates the business
rules which they believe are being enforced).  A tuple freeze at
the wrong moment could currently break that.  The patch allows the
guarantee to hold.  The fact that this bug is only seen if there is
a very-long-running transaction or if VACUUM FREEZE is run while
data-modifying transactions are active does not eliminate the fact
that without this patch data can be corrupted.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> On 2013-10-07 06:44:19 -0700, Kevin Grittner wrote:
>>
>>> Patch attached.  Any objections to applying that Real Soon Now?
>>> (When, exactly is the deadline to make today's minor release
>>> cut-off?)
>>
>> Maybe it's overly careful, but I personally slightly vote for
>> applying it after the backbranch releases. The current behaviour
>> doesn't have any harsh consequences and mostly reproduceable in
>> artifical scenarios and the logic here is complex enough that we
>> might miss something.
>>
>> A day just doesn't leave much time to noticing any issues.
>
> I grant that the bug in existing production code is not likely to
> get hit very often, but it is a bug; the new isolation test shows
> the bug clearly and shows that the suggested patch fixes it.
> What tips the scales for me is that the only possible downside if
> we missed something is an occasional false positive serialization
> failure, which does not break correctness -- we try to minimize
> those for performance reasons, but the algorithm allows them and
> they currently do happen.

I am, of course, continuing to review this.

There might be a problem if someone applies this fix while any
prepared transactions are pending.  Still investigating the impact
and possible fixes.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:

> A comment somewhere would be nice to explain why we're no longer worried
> about confusing an old tuple version with a new tuple in the same slot.
> Not sure where.

For now I counted on the commit message.  Perhaps we also want to
add something to the README file?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

> There might be a problem if someone applies this fix while any
> prepared transactions are pending.  Still investigating the impact
> and possible fixes.

I found a one-line fix for this.  It tested without problem.

Pushed.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Oct 07, 2013 at 12:26:37PM +0300, Heikki Linnakangas wrote:
> When updating a tuple, CheckTargetForConflictsIn() only marks a
> conflict if the transaction holding the predicate lock overlapped
> with the updating transaction.

Ah, this is the bit I was forgetting. (I really ought to have
remembered that, but it's been a while...)

I think it's possible, then, to construct a scenario where a slot is
reused before predicate locks on the old tuple are eligible for
cleanup -- but those locks will never cause a conflict.

So I agree: it's correct to just remove the xmin from the key
unconditionally.

And this is also true:

> And if there's a hole in that thinking I can't see right now,
> the worst that will happen is some unnecessary conflicts, ie. it's
> still correct. It surely can't be worse than upgrading the lock to a
> page-level lock, which would also create unnecessary conflicts.

Dan

--
Dan R. K. Ports UW CSE http://drkp.net/

On 07.10.2013 22:35, Kevin Grittner wrote:
> Kevin Grittner<kgrittn(at)ymail(dot)com> wrote:
>
>> There might be a problem if someone applies this fix while any
>> prepared transactions are pending. Still investigating the impact
>> and possible fixes.
>
> I found a one-line fix for this. It tested without problem.

Good catch.

To fix the bug that Hannu pointed out, we also need to apply these fixes:

http://www.postgresql.org/message-id/52440266.5040708@vmware.com

- Heikki

On 07.10.2013 23:45, Heikki Linnakangas wrote:
> On 07.10.2013 22:35, Kevin Grittner wrote:
>> Kevin Grittner<kgrittn(at)ymail(dot)com> wrote:
>>
>>> There might be a problem if someone applies this fix while any
>>> prepared transactions are pending. Still investigating the impact
>>> and possible fixes.
>>
>> I found a one-line fix for this. It tested without problem.
>
> Good catch.
>
> To fix the bug that Hannu pointed out, we also need to apply these fixes:
>
> http://www.postgresql.org/message-id/52440266.5040708@vmware.com

Per a chat with Bruce, I'm going to apply that patch now to get it into
the minor releases.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
> On 07.10.2013 23:45, Heikki Linnakangas wrote:

>> To fix the bug that Hannu pointed out, we also need to apply these fixes:
>>
>> http://www.postgresql.org/message-id/52440266.5040708@vmware.com
>
> Per a chat with Bruce, I'm going to apply that patch now to get it into
> the minor releases.

Please do.  (Somehow I had it in my head that you already had done so.)

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, 2013-10-07 at 23:49 +0300, Heikki Linnakangas wrote:
> On 07.10.2013 23:45, Heikki Linnakangas wrote:
> > On 07.10.2013 22:35, Kevin Grittner wrote:
> >> Kevin Grittner<kgrittn(at)ymail(dot)com> wrote:
> >>
> >>> There might be a problem if someone applies this fix while any
> >>> prepared transactions are pending. Still investigating the impact
> >>> and possible fixes.
> >>
> >> I found a one-line fix for this. It tested without problem.
> >
> > Good catch.
> >
> > To fix the bug that Hannu pointed out, we also need to apply these fixes:
> >
> > http://www.postgresql.org/message-id/52440266.5040708@vmware.com
>
> Per a chat with Bruce, I'm going to apply that patch now to get it into
> the minor releases.

9.1 branch is broken now.

On 08.10.2013 03:25, Peter Eisentraut wrote:
> On Mon, 2013-10-07 at 23:49 +0300, Heikki Linnakangas wrote:
>>> To fix the bug that Hannu pointed out, we also need to apply these fixes:
>>>
>>> http://www.postgresql.org/message-id/52440266.5040708@vmware.com
>>
>> Per a chat with Bruce, I'm going to apply that patch now to get it into
>> the minor releases.
>
> 9.1 branch is broken now

Rats. I forgot to "git add" the latest changes while backpatching.

Committed a fix for that.

- Heikki