Heikki Linnakangas wrote:
> On 02.02.2011 19:36, Kevin Grittner wrote:
>> I really appreciate you putting this testing framework together.
>> This is clearly the right way to do it, although we also clearly
>> don't want this test in the check target at the root directory,
>> because of the run time.
>
> It would be nice to get some buildfarm members to run them though.
Maybe it could be included in the installcheck or installcheck-world
target?
> There's still this one TODO item that you commented on earlier:
>
>> Then there's one still lurking outside the new predicate* files,
>> in heapam.c. It's about 475 lines into the heap_update function
>> (25 lines from the bottom). In reviewing where we needed to
>> acquire predicate locks, this struck me as a place where we might
>> need to duplicate the predicate lock from one tuple to another,
>> but I wasn't entirely sure. I tried for a few hours one day to
>> construct a scenario which would show a serialization anomaly if I
>> didn't do this, and failed create one. If someone who understands
>> both the patch and heapam.c wants to look at this and offer an
>> opinion, I'd be most grateful. I'll take another look and see if I
>> can get my head around it better, but failing that, I'm
>> disinclined to either add locking I'm not sure we need or to
>> remove the comment that says we *might* need it there.
>
> Have you convinced yourself that there's nothing to do? If so, we
> should replace the todo comment with a comment explaining why.
I'll look at that today and tomorrow and try to resolve the issue one
way or the other.
> PageIsPredicateLocked() is currently only called by one assertion
> in RecordFreeIndexPage(). The comment in PageIsPredicateLocked()
> says something about gist vacuuming; I presume this is something
> that will be needed to implement more fine-grained predicate
> locking in GiST. But we don't have that at the moment.
Yeah. We had something close at one point which we had to back out
because it didn't cover page splits correctly in all cases. It's a
near-certainty that we can have fine-grained coverage for the GiST AM
covered with a small patch in 9.2, and I'm pretty sure we'll need
that function for it.
> The assertion in RecordFreeIndexPage() is a reasonable sanity
> check, but I'm inclined to move it to the caller instead: I don't
> think the FSM should need to access predicate lock manager, even
> for an assertion.
OK. So it looks like right now it will move to btvacuumpage(), right
before the call to RecordFreeIndexPage(), and we will need to add it
to one spot each in the GiST and GIN AMs, when we get to those.
Would you like me to do that?
-Kevin