On Wed, Mar 28, 2018 at 2:14 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
>> This code can allocate memory in a critical section, since
>> RelationSetTargetBlock() can call smgropen():
> I think you're right. I propose that we call RelationSetTargetBlock right
> after the critical section ends. That should not have any adverse side
> effect since we're still holding WRITE lock on the page, it's just an hint
> anyways and the next time we shall do a proper recheck.
But why even do it with a write buffer lock held?
The principle behind this patch is that there can only be one
non-ignorable rightmost leaf page in the index, so we can always
detect it if our RelationSetTargetBlock() became stale; this makes it
okay that it can go stale at any time, and that we don't even have a
very weak interlock. That might be fine, but ISTM that calling
RelationSetTargetBlock() with any kind of lock held is misleading. The
hint can become invalidated immediately after the buffer lock is
released anyway, so what's the point?
>> As I already pointed out, it's unclear why some of these tests are
>> used, such as P_INCOMPLETE_SPLIT():
> Previously, we agreed that P_IGNORE() is required. So I assume no issues
> there. The other tests seem required too for us to conclusively decide to
> use the cached block.
Actually, you're right that P_INCOMPLETE_SPLIT() is the only redundant
one. Theoretically, P_IGNORE() should not be required, since page
deletion cannot delete the right most page among an internal page's
children, but actually omitting the P_IGNORE() doesn't seem like an
improvement. That's probably worth a comment, though.
--
Peter Geoghegan