Re: Potential GIN vacuum bug

Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:
> On Sun, Aug 30, 2015 at 11:11 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Your earlier point about how the current design throttles insertions to
>> keep the pending list from growing without bound seems like a bigger deal
>> to worry about. I think we'd like to have some substitute for that.
>> ...

> If the goal is to not change existing behavior (like for back patching) the
> margin should be 1, always wait.

The current behavior is buggy, both as to performance and correctness,
so I'm not sure how come "don't change the behavior" should be a
requirement.

> But we would still have to deal with the
> fact that unconditional acquire attempt by the backends will cause a vacuum
> to cancel itself, which is undesirable.

Good point.

> If we define a new namespace for
> this lock (like the relation extension lock has its own namespace) then
> perhaps the cancellation code could be made to not cancel on that
> condition. But that too seems like a lot of work to backpatch.

We could possibly teach the autocancel logic to distinguish this lock type
from others without using a new namespace. That seems a bit klugy, but
maybe better than adding a new namespace. (For example, there are
probably only a couple of modes in which we take page-level locks at
present. Choosing a currently unused, but self-exclusive, mode for taking
such a lock might serve.)

> Would we bother to back-patch a theoretical bug which there is no evidence
> is triggering in the field?

What's theoretical about it? You seemed pretty sure that the issue in
http://www.postgresql.org/message-id/flat/CA+bfosGVGVQhMAa=0-mUE6cOo7dBSgAYxb-XsnR5vm-S39hpNg(at)mail(dot)gmail(dot)com
was exactly this.

> If we want to improve the current behavior rather than fix a bug, then I
> think that if the list is greater than threshold*2 and the cleaning lock is
> unavailable, what it should do is proceed to insert the tuple's keys into
> the index itself, as if fastupdate = off. That would require some major
> surgery to the existing code, as by the time it invokes the clean up, it is
> too late to not insert into the pending list.

Meh. That's introducing a whole new behavioral regime, which quite aside
from correctness bugs might introduce new performance bugs of its own.
It certainly doesn't sound like a better back-patch candidate than the
other thing.

regards, tom lane