Re: pgsql: Add deduplication to nbtree.

Hi,

On 2020-03-29 15:19:50 -0700, Peter Geoghegan wrote:
> On Sun, Mar 29, 2020 at 3:15 PM Andres Freund  wrote:
> > Is it perhaps possible to silence the warnign with somethign along the
> > lines of
> > Assert(nhtids + vacposting->ndeletedtids == BTreeTupleGetNPosting(origtuple))
> > I don't know this code, but it looks like that'd have to be true?
> > Perhaps that'd be enough to silence coverity too?
> 
> It would have to be true. It's a tautology. That is, the value of
> nhtids comes from "vacposting->ndeletedtids" and
> "BTreeTupleGetNPosting(origtuple)" anyway, and we don't mutate any of
> that state in _bt_update_posting().

Right. It doesn't look like coverity understands that currently though.


> Wouldn't it at least be necessary to Assert() something about the
> final tuple, and/or other work state?

I don't see why? The assert might not help, but if it does, I don't
think it'd need more than that check? It depends a bit on what
coverity's precise logic here is. If its ARRAY_VS_SINGLETON checker
allows index (i.e. ui) 0 but not 1, then I think the suggested assert
could help it recognize that that's unreachable.

After staring more at the coverity trace, it looks however like
ARRAY_VS_SINGLETON might not allow any array like access, not even 0?
Seems like it assumes that BTreeTupleGetNPosting(origtuple) is at least
two, and that in the first loop d < vacposting->ndeletedtids and
vacposting->deletetids[d] == i are true, but in the second iteration
assumes d < vacposting->ndeletedtids is true but
vacposting->deletetids[d] == i. Since it doesn't record a third
iteration, it seems like it ought to see ui == 0 at that point.

I'll mark it as ignored.

Greetings,

Andres Freund