Horton

Thread

Commits

  1. Remove unnecessary clauses_attnums variable

  2. Fix choose_best_statistics to check clauses individually

  1. Failed assertion clauses != NIL

    Manuel Rigger <rigger.manuel@gmail.com> — 2019-11-19T12:50:51Z

    Hi everyone,

when building PostgreSQL with -enable-cassert, executing the following
statements result in an assertion error:

CREATE TABLE t0(c0 boolean, c1 boolean, c2 boolean);
INSERT INTO t0 VALUES(FALSE, FALSE, FALSE);
CREATE STATISTICS s0 ON c0, c2 FROM t0;
ANALYZE;
SELECT * FROM t0 WHERE t0.c2 OR t0.c1 OR t0.c0;

I can reproduce this on the latest trunk version (7f33836).

Best,
Manuel

Stacktrace:

TRAP: FailedAssertion("clauses != NIL", File: "mcv.c", Line: 1551)
postgres: postgres db [local] SELECT(ExceptionalCondition+0x76)[0x5563ab44e4a6]
postgres: postgres db [local] SELECT(+0x3d6e91)[0x5563ab2e1e91]
postgres: postgres db [local]
SELECT(mcv_clauselist_selectivity+0x41)[0x5563ab2e47b1]
postgres: postgres db [local]
SELECT(statext_clauselist_selectivity+0x362)[0x5563ab2e1692]
postgres: postgres db [local]
SELECT(clauselist_selectivity+0x174)[0x5563ab227134]
postgres: postgres db [local]
SELECT(set_baserel_size_estimates+0x38)[0x5563ab22dc38]
postgres: postgres db [local] SELECT(+0x319198)[0x5563ab224198]
postgres: postgres db [local] SELECT(make_one_rel+0x100)[0x5563ab226150]
postgres: postgres db [local] SELECT(query_planner+0x15b)[0x5563ab24b3fb]
postgres: postgres db [local] SELECT(+0x34531c)[0x5563ab25031c]
postgres: postgres db [local] SELECT(subquery_planner+0xd39)[0x5563ab253389]
postgres: postgres db [local] SELECT(standard_planner+0x117)[0x5563ab254567]
postgres: postgres db [local] SELECT(pg_plan_query+0x39)[0x5563ab320469]
postgres: postgres db [local] SELECT(pg_plan_queries+0x46)[0x5563ab320556]
postgres: postgres db [local] SELECT(+0x4158cf)[0x5563ab3208cf]
postgres: postgres db [local] SELECT(PostgresMain+0x14ef)[0x5563ab32228f]
postgres: postgres db [local] SELECT(+0x38d324)[0x5563ab298324]
postgres: postgres db [local] SELECT(PostmasterMain+0xe01)[0x5563ab299241]
postgres: postgres db [local] SELECT(main+0x4b8)[0x5563aafc1048]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xeb)[0x7f128537eb6b]
postgres: postgres db [local] SELECT(_start+0x2a)[0x5563aafc10ea]
2019-11-19 13:35:55.103 CET [22654] LOG:  server process (PID 23121)
was terminated by signal 6: Aborted
2019-11-19 13:35:55.103 CET [22654] DETAIL:  Failed process was
running: SELECT * FROM t0 WHERE t0.c2 OR t0.c1 OR t0.c0;

  2. Re: Failed assertion clauses != NIL

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-11-19T13:38:39Z

    > On Tue, Nov 19, 2019 at 01:50:51PM +0100, Manuel Rigger wrote:
>
> when building PostgreSQL with -enable-cassert, executing the following
> statements result in an assertion error:
>
> CREATE TABLE t0(c0 boolean, c1 boolean, c2 boolean);
> INSERT INTO t0 VALUES(FALSE, FALSE, FALSE);
> CREATE STATISTICS s0 ON c0, c2 FROM t0;
> ANALYZE;
> SELECT * FROM t0 WHERE t0.c2 OR t0.c1 OR t0.c0;

Yes, I can reproduce it too. mcv_get_match_bitmap expects that
stat_clauses will not be empty, but looks like in this situation
stat_clauses is indeed NIL. clauselist_selectivity_simple right before
actually doesn't insist on stat_clauses being non empty, probably it's
just too strict assert.

  3. Re: Failed assertion clauses != NIL

    Daniel Gustafsson <daniel@yesql.se> — 2019-11-19T13:45:54Z

    > On 19 Nov 2019, at 14:38, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> 
>> On Tue, Nov 19, 2019 at 01:50:51PM +0100, Manuel Rigger wrote:
>> 
>> when building PostgreSQL with -enable-cassert, executing the following
>> statements result in an assertion error:
>> 
>> CREATE TABLE t0(c0 boolean, c1 boolean, c2 boolean);
>> INSERT INTO t0 VALUES(FALSE, FALSE, FALSE);
>> CREATE STATISTICS s0 ON c0, c2 FROM t0;
>> ANALYZE;
>> SELECT * FROM t0 WHERE t0.c2 OR t0.c1 OR t0.c0;
> 
> Yes, I can reproduce it too. mcv_get_match_bitmap expects that
> stat_clauses will not be empty, but looks like in this situation
> stat_clauses is indeed NIL. clauselist_selectivity_simple right before
> actually doesn't insist on stat_clauses being non empty, probably it's
> just too strict assert.

I might be missing something, but if the clause list is NIL, wouldn't it better
to exit earlier from statext_mcv_clauselist_selectivity rather than relax the
Assertion since we will get a 1.0 estimate either way? 

cheers ./daniel

--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1267,6 +1267,9 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
                listidx++;
        }

+       if (stat_clauses == NIL)
+               return 1.0;
+
        /*
         * First compute "simple" selectivity, i.e. without the extended
         * statistics, and essentially assuming independence of the

  4. Re: Failed assertion clauses != NIL

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-11-19T14:38:42Z

    > On Tue, Nov 19, 2019 at 02:45:54PM +0100, Daniel Gustafsson wrote:
> > On 19 Nov 2019, at 14:38, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> >
> >> On Tue, Nov 19, 2019 at 01:50:51PM +0100, Manuel Rigger wrote:
> >>
> >> when building PostgreSQL with -enable-cassert, executing the following
> >> statements result in an assertion error:
> >>
> >> CREATE TABLE t0(c0 boolean, c1 boolean, c2 boolean);
> >> INSERT INTO t0 VALUES(FALSE, FALSE, FALSE);
> >> CREATE STATISTICS s0 ON c0, c2 FROM t0;
> >> ANALYZE;
> >> SELECT * FROM t0 WHERE t0.c2 OR t0.c1 OR t0.c0;
> >
> > Yes, I can reproduce it too. mcv_get_match_bitmap expects that
> > stat_clauses will not be empty, but looks like in this situation
> > stat_clauses is indeed NIL. clauselist_selectivity_simple right before
> > actually doesn't insist on stat_clauses being non empty, probably it's
> > just too strict assert.
>
> I might be missing something, but if the clause list is NIL, wouldn't it better
> to exit earlier from statext_mcv_clauselist_selectivity rather than relax the
> Assertion since we will get a 1.0 estimate either way?
>
> cheers ./daniel
>
> --- a/src/backend/statistics/extended_stats.c
> +++ b/src/backend/statistics/extended_stats.c
> @@ -1267,6 +1267,9 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
>                 listidx++;
>         }
>
> +       if (stat_clauses == NIL)
> +               return 1.0;
> +
>         /*
>          * First compute "simple" selectivity, i.e. without the extended
>          * statistics, and essentially assuming independence of the
>

Yep, seems like a reasonable thing to do, especially since it's already
like that in a few other places in this function.

  5. Re: Failed assertion clauses != NIL

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-19T15:08:40Z

    On Tue, Nov 19, 2019 at 02:45:54PM +0100, Daniel Gustafsson wrote:
>> On 19 Nov 2019, at 14:38, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>>
>>> On Tue, Nov 19, 2019 at 01:50:51PM +0100, Manuel Rigger wrote:
>>>
>>> when building PostgreSQL with -enable-cassert, executing the following
>>> statements result in an assertion error:
>>>
>>> CREATE TABLE t0(c0 boolean, c1 boolean, c2 boolean);
>>> INSERT INTO t0 VALUES(FALSE, FALSE, FALSE);
>>> CREATE STATISTICS s0 ON c0, c2 FROM t0;
>>> ANALYZE;
>>> SELECT * FROM t0 WHERE t0.c2 OR t0.c1 OR t0.c0;
>>
>> Yes, I can reproduce it too. mcv_get_match_bitmap expects that
>> stat_clauses will not be empty, but looks like in this situation
>> stat_clauses is indeed NIL. clauselist_selectivity_simple right before
>> actually doesn't insist on stat_clauses being non empty, probably it's
>> just too strict assert.
>
>I might be missing something, but if the clause list is NIL, wouldn't it better
>to exit earlier from statext_mcv_clauselist_selectivity rather than relax the
>Assertion since we will get a 1.0 estimate either way?
>

Hmmm, this is actually a thinko in how we match stats to clauses.
We simply extract attnums from Vars in each clause, and then pick the
statistic matching at least two of those attnums (and we pick the one
matching the most attnums, but that does not matter here).

And then we go and pick all the clauses covered by the statistic,
assuming that we'll get some matching clauses. Unfortunately, that fails
here because it's essentially just a single OR clause. And it references
attributes that are not covered by the statistic.

So we get clauses_attnums = {1,2,3}, we pick the statistic which however
only covers {1,3}, and then we fail because the clause is

   c0 OR c1 OD c2

which is not actually covered by the statistic, because of c1. Kaboom!

Yes, adding the condition to statext_mcv_clauselist_selectivity() would
make this go away, and it's about the  simplest solution.

Ideally, we'd be able to improve the statistics matching to recognize
it has to match all three attributes to match the clause, which in this
case would mean the OR clause is passed to clause_selectivity, and we do
some magic with extended statistics there.

I'll see how complex / backpatchable that would be.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: Failed assertion clauses != NIL

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-11-21T12:29:39Z

    On Tue, 19 Nov 2019 at 15:08, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>
> Yes, adding the condition to statext_mcv_clauselist_selectivity() would
> make this go away, and it's about the  simplest solution.
>

It's probably worth going a little further, and verifying that
stat_clauses references at least two attributes. We do that further up
for the original clause list, but it may not be true for the filtered
list. For example, given a WHERE clause like

  c0 > 0 AND c0 < 10 AND (c0 = 0 OR c1 = 1 OR c2 = 2)

and stats on (c0, c1), stat_clauses would include the first 2 clauses,
but they only reference 1 column, so it would be preferable to not use
the multivariate stats in that case.


> Ideally, we'd be able to improve the statistics matching to recognize
> it has to match all three attributes to match the clause, which in this
> case would mean the OR clause is passed to clause_selectivity, and we do
> some magic with extended statistics there.
>
> I'll see how complex / backpatchable that would be.
>

Yes, that seems like a worthwhile thing to do, but I think it goes
beyond what would normally be back-patched. It would really be a
feature enhancement rather than a bug fix.

Regards,
Dean

  7. Re: Failed assertion clauses != NIL

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-21T14:53:08Z

    On Thu, Nov 21, 2019 at 12:29:39PM +0000, Dean Rasheed wrote:
>On Tue, 19 Nov 2019 at 15:08, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> Yes, adding the condition to statext_mcv_clauselist_selectivity() would
>> make this go away, and it's about the  simplest solution.
>>
>
>It's probably worth going a little further, and verifying that
>stat_clauses references at least two attributes. We do that further up
>for the original clause list, but it may not be true for the filtered
>list. For example, given a WHERE clause like
>
>  c0 > 0 AND c0 < 10 AND (c0 = 0 OR c1 = 1 OR c2 = 2)
>
>and stats on (c0, c1), stat_clauses would include the first 2 clauses,
>but they only reference 1 column, so it would be preferable to not use
>the multivariate stats in that case.
>

Yeah, good point.

Not sure what to do about this:

   (c0 > 0 OR c2 < 10) AND (c0 = 0 OR c1 = 1 OR c2 = 2)

In that case we match just the first OR clause, but it references two
attributes. I guess we should ignore that too and handle that later just
like the regular OR clauses.

>
>> Ideally, we'd be able to improve the statistics matching to recognize
>> it has to match all three attributes to match the clause, which in this
>> case would mean the OR clause is passed to clause_selectivity, and we do
>> some magic with extended statistics there.
>>
>> I'll see how complex / backpatchable that would be.
>>
>
>Yes, that seems like a worthwhile thing to do, but I think it goes
>beyond what would normally be back-patched. It would really be a
>feature enhancement rather than a bug fix.
>

True. I don't have a patch yet, but it's likely to be a bit more
invasive than I'd like to backpatch.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: Failed assertion clauses != NIL

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-24T23:40:11Z

    Hi,

Attached are two patched, related to this bug report.


0001 - Fix choose_best_statistics to check clauses individually
---------------------------------------------------------------

This modifies the choose_best_statistics function to properly check
which clauses are actually covered by each statistic object, and only
use attnums from those.

The patch ended up pretty small, because we already have all the
necessary info (per-clause attnums) precalculated. Which means this
should not be much more expensive than before.

The main drawback is that this does change signature of a function
defined in statistics.h - we have to pass more info (per-clause bitmaps
and info which clauses are already estimated). Which means ABI break.

I'm not sure how likely it is that external code is calling this
function, but the probability is non-zero. So maybe even if the patch is
fairly small, in backbranches we should use the simple fix with just
returning if the list is NIL.


0002 - WIP: Use extended statistics to estimate OR clauses
----------------------------------------------------------

No matter what 0001 does, it's clear the current code fails to handle OR
clauses that are not fully covered by an extended statistic. For AND
clauses that's not an issue - we simply estimate the covered ones, and
then add the remaining ones by assuming independence.

But clauselist_selectivity sees OR clauses as a single single clause,
and clause_selectivity() simply used the (s1+s2-s1*s2) formula without
considering extended statistics for is_orclause. (For is_andclause we
call clauselist_selectivity recursively, which does consider extended
stats, of course.)

This commit addresses this by calling a clauselist_selectivity variant
for clauses connected by OR, and calling it from the is_orclause branch.
It then requires a bunch of changes elsewhere, to propagate the is_or
flag properly etc.

This is clearly not a thing we could/want to backpatch, and at this
point it's not anywhere close to committable. It's more a WIP patch
highlighting places that will need tweaking to make this work.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: Failed assertion clauses != NIL

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-11-26T10:56:33Z

    On Sun, 24 Nov 2019 at 23:40, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>
> Hi,
>
> Attached are two patched, related to this bug report.
>
>
> 0001 - Fix choose_best_statistics to check clauses individually
> ---------------------------------------------------------------
>
> This modifies the choose_best_statistics function to properly check
> which clauses are actually covered by each statistic object, and only
> use attnums from those.
>
> The patch ended up pretty small, because we already have all the
> necessary info (per-clause attnums) precalculated. Which means this
> should not be much more expensive than before.
>
> The main drawback is that this does change signature of a function
> defined in statistics.h - we have to pass more info (per-clause bitmaps
> and info which clauses are already estimated). Which means ABI break.
>
> I'm not sure how likely it is that external code is calling this
> function, but the probability is non-zero. So maybe even if the patch is
> fairly small, in backbranches we should use the simple fix with just
> returning if the list is NIL.
>

On a quick read-through that algorithm makes a lot more sense. It
seems pretty unlikely that anyone would be using
choose_best_statistics() anywhere else, so I think maybe it's fine to
change that in back-branches.

A couple of comments:

1). I think you should pass estimatedClauses to
choose_best_statistics() as a pure input parameter (i.e., remove one
"*"), since it doesn't (and must not) modify that set. 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.

2). The new parameter "clauses" should probably be called something
like "clause_attnums" or some such, to better reflect what it actually
is. And it should be documented that NULL values represent
incompatible/already-estimated clauses.

3). In statext_mcv_clauselist_selectivity(), the check for at least 2
attributes is no longer needed, because choose_best_statistics() now
does that, so there's also no need to compute clauses_attnums there.

Regards,
Dean

  10. Re: Failed assertion clauses != NIL

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-26T17:41:31Z

    On Tue, Nov 26, 2019 at 10:56:33AM +0000, Dean Rasheed wrote:
>On Sun, 24 Nov 2019 at 23:40, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> Hi,
>>
>> Attached are two patched, related to this bug report.
>>
>>
>> 0001 - Fix choose_best_statistics to check clauses individually
>> ---------------------------------------------------------------
>>
>> This modifies the choose_best_statistics function to properly check
>> which clauses are actually covered by each statistic object, and only
>> use attnums from those.
>>
>> The patch ended up pretty small, because we already have all the
>> necessary info (per-clause attnums) precalculated. Which means this
>> should not be much more expensive than before.
>>
>> The main drawback is that this does change signature of a function
>> defined in statistics.h - we have to pass more info (per-clause bitmaps
>> and info which clauses are already estimated). Which means ABI break.
>>
>> I'm not sure how likely it is that external code is calling this
>> function, but the probability is non-zero. So maybe even if the patch is
>> fairly small, in backbranches we should use the simple fix with just
>> returning if the list is NIL.
>>
>
>On a quick read-through that algorithm makes a lot more sense. It
>seems pretty unlikely that anyone would be using
>choose_best_statistics() anywhere else, so I think maybe it's fine to
>change that in back-branches.
>
>A couple of comments:
>
>1). I think you should pass estimatedClauses to
>choose_best_statistics() as a pure input parameter (i.e., remove one
>"*"), since it doesn't (and must not) modify that set.

Yeah, good point.

>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.

>2). The new parameter "clauses" should probably be called something
>like "clause_attnums" or some such, to better reflect what it actually
>is. And it should be documented that NULL values represent
>incompatible/already-estimated clauses.
>

Yes, agreed.

>3). In statext_mcv_clauselist_selectivity(), the check for at least 2
>attributes is no longer needed, because choose_best_statistics() now
>does that, so there's also no need to compute clauses_attnums there.
>

Good point. I'll replace that with an assert.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: Failed assertion clauses != NIL

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-27T00:33:11Z

    On Tue, Nov 26, 2019 at 06:41:31PM +0100, Tomas Vondra wrote:
>On Tue, Nov 26, 2019 at 10:56:33AM +0000, Dean Rasheed wrote:
>>On Sun, 24 Nov 2019 at 23:40, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>>
>>>Hi,
>>>
>>>Attached are two patched, related to this bug report.
>>>
>>>
>>>0001 - Fix choose_best_statistics to check clauses individually
>>>---------------------------------------------------------------
>>>
>>>This modifies the choose_best_statistics function to properly check
>>>which clauses are actually covered by each statistic object, and only
>>>use attnums from those.
>>>
>>>The patch ended up pretty small, because we already have all the
>>>necessary info (per-clause attnums) precalculated. Which means this
>>>should not be much more expensive than before.
>>>
>>>The main drawback is that this does change signature of a function
>>>defined in statistics.h - we have to pass more info (per-clause bitmaps
>>>and info which clauses are already estimated). Which means ABI break.
>>>
>>>I'm not sure how likely it is that external code is calling this
>>>function, but the probability is non-zero. So maybe even if the patch is
>>>fairly small, in backbranches we should use the simple fix with just
>>>returning if the list is NIL.
>>>
>>
>>On a quick read-through that algorithm makes a lot more sense. It
>>seems pretty unlikely that anyone would be using
>>choose_best_statistics() anywhere else, so I think maybe it's fine to
>>change that in back-branches.
>>
>>A couple of comments:
>>
>>1). I think you should pass estimatedClauses to
>>choose_best_statistics() as a pure input parameter (i.e., remove one
>>"*"), since it doesn't (and must not) modify that set.
>
>Yeah, good point.
>
>>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.

Attached is the 0001 part, addressing (hopefully) all the comments.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: Failed assertion clauses != NIL

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2019-11-27T09:26:11Z

    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.

Regards,
Dean

  13. Re: Failed assertion clauses != NIL

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-11-28T21:40:30Z

    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