Alvaro Herrera <alvherre@commandprompt.com> writes:
> SIBackendInit returns a flag indicating whether it worked or not. Since
> there is only one caller and it dies with a FATAL error when
> SIBackendInit failed, it seems better to move the elog and remove the
> return value, per this patch.
The reason for the existing coding is to release the SInvalLock before
throwing the error. Now proc_exit cleanup should release the lock
anyway, but this proposed change will mean that a failing backend holds
the lock a bit longer before releasing, which might be a bad thing.
A more severe consequence would be if the proc_exit sequence tried to
grab SInvalLock again before getting to LWLockReleaseAll. I think
that's not possible but I'm not 100% certain. In particular it's a bit
scary that CleanupInvalidationState is registered with on_proc_exit
while still holding the lock; maybe we should change that? If an error
were to be thrown at just that instant, it *would* fail because ProcKill
will be further down the on_proc_exit list than CleanupInvalidationState.
On the whole it doesn't seem worth messing with, or at least not to just
this extent. If you want to refactor this, I'd suggest that management
of the SInvalLock should be moved over to the sinvaladt.c side of the
fence altogether. It seems a bit bogus that CleanupInvalidationState
knows about the lock while the other routines in that file don't.
(Actually, the division of responsibility between sinval.c and
sinvaladt.c has never been real clean AFAICS ... so I think if you want
to touch this you should try to make that division better-specified.)
regards, tom lane
