From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Andreas Seltenreich <seltenreich@gmx.de>
Cc: pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@postgresql.org>
Subject: Re: [sqlsmith] Crash in mcv_get_match_bitmap
Date: Thu, 18 Jul 2019 11:16:08 -0400
Message-ID: <23667.1563462968@sss.pgh.pa.us> (raw)
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>
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 code
> 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 for
> 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, because
* it does not matter whether it's (var op const)
* or (const op var).
*/
match = DatumGetBool(FunctionCall2Coll(&opproc,
DEFAULT_COLLATION_OID,
cst->constvalue,
item->values[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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, tomas.vondra@2ndquadrant.com, seltenreich@gmx.de, tomas.vondra@postgresql.org
Subject: Re: [sqlsmith] Crash in mcv_get_match_bitmap
In-Reply-To: <23667.1563462968@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox