Re: [sqlsmith] Crash in mcv_get_match_bitmap
Tomas Vondra <tomas.vondra@2ndquadrant.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andreas Seltenreich <seltenreich@gmx.de>, pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@postgresql.org>
Date: 2019-07-18T11:20:01Z
Lists: pgsql-hackers
Attachments
Hi, I've pushed the fixes listed in the previous message, with the exception of the collation part, because I had some doubts about that. 1) handling of NULL values in Cons / MCV items The handling of NULL elements was actually a bit more broken, because it also was not quite correct for NULL values in the MCV items. The code treated this as a mismatch, but then skipped the rest of the evaluation only for AND-clauses (because then 'false' is final). But for OR-clauses it happily proceeded to call the proc, etc. It was not hard to cause a crash with statistics on varlena columns. I've fixed this and added a simple regression test to check this. It however shows the stats_ext suite needs some improvements - until now it only had AND-clauses. Now it has one simple OR-clause test, but it needs more of that - and perhaps some combinations mixing AND/OR. I've tried adding an copy of each existing query, with AND replaced by OR, and that works fine (no crashes, estimates seem OK). But it's quite heavy-handed way to create regression tests, so I'll look into this in PG13 cycle. 2) collations 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). And we reset stats whenever the attribute type changes (which includes change of collation for the column), so I think it's OK. To be precise, we only reset MCV list in that case - we keep mvdistinct and dependencies, but that's probably OK because those don't store values and we won't run any functions on them. So I think the attached patch is OK, but I'd welcome some feedback. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Commits
-
Use column collation for extended statistics
- a63378a03ec0 13.0 landed
- 5a8ee9f2dbe0 12.0 landed
-
Rework examine_opclause_expression to use varonleft
- e38a55ba46bb 13.0 landed
- fc4faea17971 12.0 landed
-
Fix handling of NULLs in MCV items and constants
- 1c2acc32dc6c 12.0 landed
- e4deae7396f2 13.0 landed
-
Fix handling of opclauses in extended statistics
- 42276976a143 12.0 landed
- e8b6ae2130e3 13.0 landed
-
Remove unnecessary TYPECACHE_GT_OPR lookup
- 3944e855bc5b 12.0 landed
- a4303a078c66 13.0 landed
-
Simplify bitmap updates in multivariate MCV code
- 79d3a1e52a3c 12.0 landed
- 7d24f6a49076 13.0 landed
-
Make pg_statistic and related code account more honestly for collations.
- 5e09280057a4 12.0 cited