Re: Write skew observed under serializable isolation

[cc'ing hackers this time, oops!]

Hi Matthias,

Thanks for looking at this.

> I think this patch is taking locks too aggressively; we shouldn't take
> a predlock on the whole relation when we're only looking at a small
> portion of the table using TID ranges. I think the more appropriate
> place to add the predicate locks is in heap_set_tidrange(), and have
> Assert(PageIsPredicateLocked(...)) for the returned TID in
> heap_getnextslot_tidrange() to make sure we don't regress.

I tried the page-level approach, but I don't think heap page locks are
sufficient here. heap_insert() checks only for relation-level predicate
locks; heap page locks are treated as consolidated tuple locks and do not
cover gaps. Thus an insert can land within the scanned TID range, even on a
page covered by a page lock, without conflicting with the reader.

There is also a problem for ranges extending beyond the current end of the
relation. heap_set_tidrange() limits the scan using the relation's current
size, so it cannot lock pages subsequently added by an insert even when
those pages fall within the requested TID range.

The proposed assertion in heap_getnextslot_tidrange() also does not appear
safe after predicate-lock promotion. PageIsPredicateLocked() intentionally
checks only for a page-level lock, not a covering relation-level lock,
while acquiring enough page locks can promote them to a relation lock.

It seems that using page locks here would require some additional notion of
heap gap locking, or corresponding changes to the insert conflict checks.
With the existing predicate-locking scheme, I think the relation lock is
needed to cover inserts as well as updates.

Best,

Jacob