Andres Freund <[email protected]> writes:
> I think this is more likely to be a spgist bug, not a bug in the patch. From
> what I can tell, spgist tries to conditionally lock a buffer that it itself
> already has locked exclusively - that's why the assertion is failing.
I dunno. It looks to me like the previous LWLock-based implementation
of ConditionalLockBuffer() had no such restriction as
/*
* We better not already hold a lock on the buffer.
*/
Assert(entry->data.lockmode == BUFFER_LOCK_UNLOCK);
Maybe I'm missing something, but it looks like the old code would
return false if the buffer was already locked, whether that lock
was held by our process or another one. SPGist evidently has an
assumption in edge cases that that is the behavior, and I'm not
convinced that it's a good idea to change it. There may be other
such edge cases we've not tripped over yet.
> We could of course just accept this case and have the conditional lock
> acquisition fail, but I think trying to conditionally lock a buffer that you
> already lock is indicative of something having gone wrong.
I don't really buy this argument. Yes, within a single function it'd
be silly to lock a buffer and immediately try to lock it again, but
when you consider cases like recursive modifications of index state,
it's *far* from obvious that some lower recursion level might not try
to lock a buffer that some outer level already locked. In the case at
hand I think it is probably driven by two recursion levels trying to
acquire free space out of the same buffer. SPGist is expecting the
lower level to fail to get the lock and then go find some free space
elsewhere. Yeah, we could probably re-code it to get that outcome
in another way, but why?
regards, tom lane
