[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
