On Wed, Nov 27, 2019 at 09:26:11AM +0000, Dean Rasheed wrote:
>On Wed, 27 Nov 2019 at 00:33, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> >On Tue, Nov 26, 2019 at 10:56:33AM +0000, Dean Rasheed wrote:
>> >
>> >>In fact, on closer inspection, I don't think you need to pass it to
>> >>choose_best_statistics() at all, since its callers already check
>> >>clauses against estimatedClauses. Therefore, in
>> >>choose_best_statistics(), incompatible and already-estimated clauses
>> >>both appear the same (as NULL/empty attribute sets), and therefore the
>> >>estimatedClauses check will never be tripped.
>> >
>> >Right, but I'm thinking about the patch that allows applying multiple
>> >statistics. With that applied, this changes to a while loop - and we'll
>> >either have to rebuild the list_attnums or pass the bitmap.
>>
>> On second thought, that's not quite relevant in backbranches, so I've
>> removed the parameters for now. I'll add it in the patch that adds
>> support for multiple stats.
>>
>
>Or alternatively, that patch could just NULL-out the relevant
>list_attnums[] array entry once the corresponding clause has been
>estimated, which would avoid needing to change
>choose_best_statistics() again.
>
>> Attached is the 0001 part, addressing (hopefully) all the comments.
>>
>
>I just spotted a trivial comment typo in dependencies_clauselist_selectivity():
>
>+ /*
>+ * We expect the bitmaps ton contain a single attribute number.
>+ */
>+ attnum = bms_singleton_member(list_attnums[listidx]);
>
>s/ton/to/
>
>Also, in statext_mcv_clauselist_selectivity(), clauses_attnums is now
>unused, so there's no point in computing it (unless you wanted to add
>the Assert() you talked about, but I don't think it's really
>necessary).
>
>Otherwise it looks good to me.
>
I've pushed this, after adding a regression test for OR clauses that are
not fully covered by the MCV statistic. The existing regression queries
test almost exclusively AND clauses, so I've considered adding a couple
more, but then I reliazed the support for OR clauses is somewhat limited
so the patch improving support for OR clauses is a better opportunity.
And now that I look at Dean's message again, I realize I forgot to
remove the unnecessary clauses_attnum variable, so I'll fix that.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services