Date: Sun, 29 Mar 2020 15:41:50 -0700
From: Andres Freund <andres@anarazel.de>
To: Peter Geoghegan <pg@bowt.ie>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-committers <pgsql-committers@lists.postgresql.org>
Subject: Re: pgsql: Add deduplication to nbtree.
Message-ID: <20200329224150.p2kmwqzmrdsej7iw@alap3.anarazel.de>
References: <27003.1583087095@sss.pgh.pa.us>
 <CAH2-Wzn9_zQR9Q96JUyVBAdro52Ov6+bQ1zcE_FPxEVXGw-ebQ@mail.gmail.com>
 <29676.1583090977@sss.pgh.pa.us>
 <CAH2-WzmAufHiOku6AGiFD=81VQs5nYJ1L2YkhW1t+BH4CMsgRw@mail.gmail.com>
 <CAH2-Wz=Tz_e8e39qTs3Mt78P6XD9s3u7TdOUVrYZg2eTEgfB0w@mail.gmail.com>
 <5838.1583100843@sss.pgh.pa.us>
 <CAH2-WzkPR8KMwkL0ap976kmXwBCeukTeHz6fB-U__wvuP1S9Zg@mail.gmail.com>
 <CAH2-WznRH9zmUn3tthVgHp3KDNvqCEL-E2u+xTgQYeT2xtfHqw@mail.gmail.com>
 <20200329221544.w2ihd7u3qdztnbr2@alap3.anarazel.de>
 <CAH2-Wzmpm6=smT9b75gLZ3-7ui=pBPLt46FdTa3TM=Zk3__=8Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAH2-Wzmpm6=smT9b75gLZ3-7ui=pBPLt46FdTa3TM=Zk3__=8Q@mail.gmail.com>
Precedence: bulk

Hi,

On 2020-03-29 15:19:50 -0700, Peter Geoghegan wrote:
> On Sun, Mar 29, 2020 at 3:15 PM Andres Freund <andres@anarazel.de> 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