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.
I'm reading through the patch again now, hoping to commit this weekend.
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.
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. 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.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com