From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
cc: Andreas Seltenreich <seltenreich@gmx.de>,
        pgsql-hackers@lists.postgresql.org,
        Tomas Vondra <tomas.vondra@postgresql.org>
Subject: Re: [sqlsmith] Crash in mcv_get_match_bitmap
In-reply-to: <20190718112001.gtnquw24f4qzxia5@development>
References: <15097.1562792274@sss.pgh.pa.us>
 <20190710212609.5mrl7hltbozoxpq7@development>
 <17042.1562795124@sss.pgh.pa.us>
 <20190710215904.ncaqlswrylliisv3@development>
 <19586.1562798896@sss.pgh.pa.us>
 <20190711150822.z7cirna2fjtrfcnu@development>
 <20190713001137.7saj5g46qpdump4r@development>
 <19208.1563032395@sss.pgh.pa.us>
 <20190713204342.pysg4odqj2j6gn3o@development>
 <20190715013425.v2hqfq5ubcbslw67@development>
 <20190718112001.gtnquw24f4qzxia5@development>
Comments: In-reply-to Tomas Vondra <tomas.vondra@2ndquadrant.com>
	message dated "Thu, 18 Jul 2019 13:20:01 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <23666.1563462968.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 18 Jul 2019 11:16:08 -0400
Message-ID: <23667.1563462968@sss.pgh.pa.us>
Precedence: bulk

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> I've pushed the fixes listed in the previous message, with the exception
> of the collation part, because I had some doubts about that.

Sorry for being slow on this.

> Now, for the collation part - after some more thought and looking at cod=
e
> I think the fix I shared before is OK. It uses per-column collations
> consistently both when building the stats and estimating clauses, which
> makes it consistent. I was not quite sure if using Var->varcollid is the
> same thing as pg_attribute.attcollation, but it seems it is - at least f=
or
> Vars pointing to regular columns (which for extended stats should always
> be the case).

I think you are right, but it could use some comments in the code.
The important point here is that if we coerce a Var's collation to
something else, that will be represented as a separate CollateExpr
(which will be a RelabelType by the time it gets here, I believe).
We don't just replace varcollid, the way eval_const_expressions will
do to a Const.


While I'm looking at the code --- I don't find this at all convincing:

                            /*
                             * We don't care about isgt in equality, becau=
se
                             * it does not matter whether it's (var op con=
st)
                             * or (const op var).
                             */
                            match =3D DatumGetBool(FunctionCall2Coll(&oppr=
oc,
                                                                   DEFAULT=
_COLLATION_OID,
                                                                   cst->co=
nstvalue,
                                                                   item->v=
alues[idx]));

It *will* matter if the operator is cross-type.  I think there is no
good reason to have different code paths for the equality and inequality
cases --- just look at isgt and swap or don't swap the arguments.

BTW, "isgt" seems like a completely misleading name for that variable.
AFAICS, what that is actually telling is whether the var is on the left
or right side of the OpExpr.  Everywhere else in the planner with a
need for that uses "bool varonleft", and I think you should do likewise
here (though note that that isgt, as coded, is more like "varonright").

			regards, tom lane