Thread: Safely calling index_getprocinfo() while holding an nbtree exclusivebuffer lock

My nbtree patch [1] needs to call index_getprocinfo() with an
exclusive buffer lock held during a leaf page split. This has an
undetectable self-deadlock (LWLock self-deadlock) risk: a syscache
lookup against pg_proc might have a catcache miss, ending with an
index scan that needs to access the very same buffer. That's not
acceptable.

There is very similar code to this in SP-GiST:  there is a
index_getprocinfo() call within doPickSplit(), to get the user-defined
method for a split (SPGIST_PICKSPLIT_PROC). My nbtree patch builds a
new insertion scankey to determine how many attributes we can safely
truncate away in new pivot tuples -- it would be tricky to do this
lookup outside of the split function. I suppose that it's okay to do
this in SP-GiST without special care because there cannot be an
SP-GiST index on a system catalog. I'll need to do something else
about it given that I'm doing this within nbtree, though -- I don't
want to complicate the code that deals with insertion scankeys to make
this work.

Here is a strategy that fixes the problem without complicating matters
for nbtree: It should be safe if I make a point of using a special
comparator (the bitwise one that we already use in other contexts in
the patch) with system catalog indexes. We know that they cannot be of
types that have a varlena header + typstorage != 'p', which ensures
that there are no cases where bitwise equality fails to be a reliable
indicator of opclass equality (e.g. there are no cases like numeric
display scale). We could make sure that this requirement isn't
violated in the future by adding a pg_index test to opr_sanity.sql,
limiting system catalog indexes to opclasses that are known-safe for
the bitwise comparator.

Does that seem sane?

[1] https://commitfest.postgresql.org/21/1787/
-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> My nbtree patch [1] needs to call index_getprocinfo() with an
> exclusive buffer lock held during a leaf page split.

I think you should stop right there and ask why.  Surely that info
can be obtained before starting the operation?  Quite aside from the
deadlock hazard, I do not think holding an exclusive buffer lock
for long enough to go consult a system catalog will be acceptable
from a performance/concurrency standpoint.

            regards, tom lane

On Mon, Jan 14, 2019 at 7:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think you should stop right there and ask why.  Surely that info
> can be obtained before starting the operation?

*Thinks some more*

Uh, I'm already telling the same _bt_truncate() code path that it is
being called from a CREATE INDEX, allowing it to avoid accessing the
metapage. I now think that it would be perfectly acceptable to just
pass down the insertion scan key for the tuple that caused the split,
instead of that build bool, and handle both deadlock issues
(index_getprocinfo() hazard and metapage hazard) that way instead.
Heikki expressed some concerns about the way the patch accesses the
metapage already.

I jumped the gun with this catalog index business. Clearly I'd be much
better off avoiding *all* new buffer lock protocol stuff by getting
both pieces of information up-front -- for some reason I thought that
that would be harder than it now appears.

Thanks
--
Peter Geoghegan