Re: Another bug introduced by fastpath patch

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> In terms of making this more robust, one idea - along the lines you
> mentioned in your original post - is to have a separate code path for
> the case where we're releasing *all* locks.

Yeah, I thought seriously about that, but concluded that it was too
debatable for a back-patch. The potential robustness gain from having
guaranteed lock clearing might be offset by the potential for bugs
in one or the other of separate code paths. Also, we're not going to
get very far unless we refactor LockReleaseAll so that we're not making
two separate DEFAULT_LOCKMETHOD and USER_LOCKMETHOD calls in every
transaction end. (Or maybe we could fix things so that we remember
if we have any USER locks, and skip the second call if not?)

BTW, after further thought I can refine the argument why this patch is
needed. The risk is that we might fetch the list pointer while someone
else is adding formerly-fastpath locks to that list. Now, by the same
argument as before, any such just-added entries aren't ones that we need
to visit, so starting at the old list head isn't really a problem.
(Though it might become one if we ever switch to a different list
traversal technology here.) The only remaining risk is that, if pointer
fetch/store isn't atomic, we might fetch a half-updated pointer; which
will be non-null, but not something we can use to reach the list. Since
we do purport to support such architectures, we'd better apply the patch.
I'll change the comment a bit to mention this.

regards, tom lane