Re: Failed assertion clauses != NIL

On Wed, 27 Nov 2019 at 00:33, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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.

Regards,
Dean