RE: Showing applied extended statistics in explain Part 2
> On 7/18/24 12:37, masahiro.ik...@nttdata.com wrote:
> >> Let me share my opinion on those questions ...
> > ...>
> >> For ndistinct, I think we don't show this because it doesn't go
> >> through clauselist_selectivity, which is the only thing I modified in the
> >> PoC.
> >> But I guess we might improve estimate_num_groups() to track the stats
> >> in a similar way, I guess.
> >
> > Thanks. IIUC, the reason is that it doesn't go through
> > statext_clauselist_selectivity() because the number of clauses is one
> > though it goes
> through clauselist_selectivity().
> >
>
> Ah, I see I misunderstood the original report. The query used was
>
> EXPLAIN (STATS, ANALYZE) SELECT * FROM t3
> WHERE date_trunc('month', a) = '2020-01-01'::timestamp;
>
> And it has nothing to do with the number of clauses being one neither.
>
> The problem is this estimate is handled by examine_variable() matching the
> expression
> to the "expression" stats, and injecting it into the variable, so that the
> clauselist_selectivity() sees these stats.
>
> This would happen even if you build just expression statistics on each of the
> date_trunc() calls, and then tried a query with two clauses:
>
> CREATE STATISTICS s4 ON date_trunc('day', a) FROM t3;
> CREATE STATISTICS s3 ON date_trunc('month', a) FROM t3;
>
> EXPLAIN SELECT * FROM t3
> WHERE date_trunc('month', a) = '2020-01-01'::timestamp
> AND date_trunc('day', 'a') = '2020-01-01'::timestamp;
>
> Not sure how to handle this - we could remember when explain_variable()
> injects
> statistics like this, I guess. But do we know that each call to
> examine_variable() is for estimation? And do we know for which clause?
I see. The issue is related to extended statistics for single expression. As a
first step, it's ok for me that we don't support it.
The below is just an idea to know clauses...
Although I'm missing something, can callers of examine_variable()
for estimation to rebuild the clauses from partial information of "OpExpr"?
Only clause_selectivity_ext() knows the information of actual full clauses.
But we don't need full information. It's enough to know the information
to show "OpExpr" for EXPLAIN.
get_oper_expr() deparse "OpExpr" using only the operator oid and arguments
in get_oper_expr().
If so, the caller to estimate, for example eqsel_internal(),
scalarineqsel_wrapper()
and so on, seems to be able to know the "OpExpr" information, which are operator
oid and arguments, and used extended statistics easily to show for EXPLAIN.
# Memo: the call path of the estimation function
caller to estimate selectivity
(eqsel_internal()/scalargtjoinsel_wrappter()/...)
-> get_restriction_variable()/get_join_valiables()
-> examine_variable()
> >>> ERROR: unrecognized node type: 268
> >
> > Regarding the above error, do "applied_stats" need have the list of
> > "StatisticExtInfo"
> > because it's enough to have the list of Oid(stat->statOid) for EXPLAIN
> > output in the
> current patch?
> > change_to_applied_stats_has_list_of_oids.diff is the change I assumed.
> > Do you have any plan to show extra information for example "kind" of
> "StatisticExtInfo"?
> >
> > The above is just one idea came up with while I read the following
> > comments of header of pathnodes.h, and to support copy "StatisticExtInfo"
> > will leads
> many other nodes to support copy.
> > * We don't support copying RelOptInfo, IndexOptInfo, or Path nodes.
> > * There are some subsidiary structs that are useful to copy, though.
> >
>
> I do think tracking just the OID would work, because we already know how to
> copy List
> objects. But if we want to also track the clauses, we'd have to keep multiple
> lists, right?
> That seems a bit inconvenient.
Understood. In future, we might show not only the applied_clauses but also the
clauses of
its extended statistics (StatisticExtInfo->exprs).
> > By the way, I found curios result while I tested with the above patch. It
> > shows same
> "Ext Stats" twice.
> > I think it's expected behavior because the stat is used when estimate the
> > cost of
> "Partial HashAggregate" and "Group".
> > I've shared the result because I could not understand soon when I saw
> > it first time. I think it's better to let users understand when the stats
> > are used, but I
> don't have any idea now.
> >
> > -- I tested with the example of CREATE STATISTICS documentation.
> > psql=# EXPLAIN (STATS, ANALYZE) SELECT date_trunc('month', a),
> > date_trunc('day',
> a) FROM t3 GROUP BY 1, 2;
> > QUERY PLAN
> > --
> > --
> > - Group (cost=9530.56..9576.18 rows=365 width=16) (actual
> > time=286.908..287.909 rows=366 loops=1)
> >Group Key: (date_trunc('month'::text, a)), (date_trunc('day'::text, a))
> >-> Gather Merge (cost=9530.56..
Re: Showing applied extended statistics in explain Part 2
On 7/18/24 12:37, masahiro.ik...@nttdata.com wrote:
>> Let me share my opinion on those questions ...
> ...>
>> For ndistinct, I think we don't show this because it doesn't go through
>> clauselist_selectivity, which is the only thing I modified in the PoC.
>> But I guess we might improve estimate_num_groups() to track the stats in a
>> similar way,
>> I guess.
>
> Thanks. IIUC, the reason is that it doesn't go through
> statext_clauselist_selectivity() because
> the number of clauses is one though it goes through clauselist_selectivity().
>
Ah, I see I misunderstood the original report. The query used was
EXPLAIN (STATS, ANALYZE) SELECT * FROM t3
WHERE date_trunc('month', a) = '2020-01-01'::timestamp;
And it has nothing to do with the number of clauses being one neither.
The problem is this estimate is handled by examine_variable() matching
the expression to the "expression" stats, and injecting it into the
variable, so that the clauselist_selectivity() sees these stats.
This would happen even if you build just expression statistics on each
of the date_trunc() calls, and then tried a query with two clauses:
CREATE STATISTICS s4 ON date_trunc('day', a) FROM t3;
CREATE STATISTICS s3 ON date_trunc('month', a) FROM t3;
EXPLAIN SELECT * FROM t3
WHERE date_trunc('month', a) = '2020-01-01'::timestamp
AND date_trunc('day', 'a') = '2020-01-01'::timestamp;
Not sure how to handle this - we could remember when explain_variable()
injects statistics like this, I guess. But do we know that each call to
examine_variable() is for estimation? And do we know for which clause?
>
>>> ERROR: unrecognized node type: 268
>
> Regarding the above error, do "applied_stats" need have the list of
> "StatisticExtInfo"
> because it's enough to have the list of Oid(stat->statOid) for EXPLAIN output
> in the current patch?
> change_to_applied_stats_has_list_of_oids.diff is the change I assumed. Do you
> have any plan to
> show extra information for example "kind" of "StatisticExtInfo"?
>
> The above is just one idea came up with while I read the following comments
> of header
> of pathnodes.h, and to support copy "StatisticExtInfo" will leads many other
> nodes to support copy.
> * We don't support copying RelOptInfo, IndexOptInfo, or Path nodes.
> * There are some subsidiary structs that are useful to copy, though.
>
I do think tracking just the OID would work, because we already know how
to copy List objects. But if we want to also track the clauses, we'd
have to keep multiple lists, right? That seems a bit inconvenient.
>
> By the way, I found curios result while I tested with the above patch. It
> shows same "Ext Stats" twice.
> I think it's expected behavior because the stat is used when estimate the
> cost of "Partial HashAggregate" and "Group".
> I've shared the result because I could not understand soon when I saw it
> first time. I think it's better to let users understand
> when the stats are used, but I don't have any idea now.
>
> -- I tested with the example of CREATE STATISTICS documentation.
> psql=# EXPLAIN (STATS, ANALYZE) SELECT date_trunc('month', a),
> date_trunc('day', a) FROM t3 GROUP BY 1, 2;
> QUERY PLAN
>
> -
> Group (cost=9530.56..9576.18 rows=365 width=16) (actual
> time=286.908..287.909 rows=366 loops=1)
>Group Key: (date_trunc('month'::text, a)), (date_trunc('day'::text, a))
>-> Gather Merge (cost=9530.56..9572.53 rows=365 width=16) (actual
> time=286.904..287.822 rows=498 loops=1)
> Workers Planned: 1
> Workers Launched: 1
> -> Sort (cost=8530.55..8531.46 rows=365 width=16) (actual
> time=282.905..282.919 rows=249 loops=2)
>Sort Key: (date_trunc('month'::text, a)),
> (date_trunc('day'::text, a))
>Sort Method: quicksort Memory: 32kB
>Worker 0: Sort Method: quicksort Memory: 32kB
>-> Partial HashAggregate (cost=8509.54..8515.02 rows=365
> width=16) (actual time=282.716..282.768 rows=249 loops=2)
> Group Key: date_trunc('month'::text, a),
> date_trunc('day'::text, a)
> Batches: 1 Memory Usage: 45kB
> Worker 0: Batches: 1 Memory Usage: 45kB
> -> Parallel Seq Scan on t3 (cost=0.00..6963.66
> rows=309177 width=16) (actual time=0.021..171.214 rows=262800 loops=2)
>Ext Stats: public.s3 Clauses:
> date_trunc('month'::text, a), date_trunc('day'::text, a) -- here
>Ext Stats: public.s3 Clauses:
> date_trunc('month'::text, a), date_trunc('day'::text, a) -- here
> Planning Time: 114327.206 ms
> Execution Time: 288.007 ms
> (18 rows)
>
RE: Showing applied extended statistics in explain Part 2
> Let me share my opinion on those questions ...
Thanks! I could understand the patch well thanks to your comments.
> On 7/12/24 12:09, masahiro.ik...@nttdata.com wrote:
> > Is it better to make the order of output consistent? For example, even
> > though there are three clauses shown in the below case, the order does
> > not match.
> > * "Filter" shows that "id1" is first.
> > * "Ext Stats" shows that "id2" is first.
> >
> > -- An example
> > DROP TABLE IF EXISTS test;
> > CREATE TABLE test (id1 int2, id2 int4, id3 int8, value varchar(32));
> > INSERT INTO test (SELECT i%11, i%103, i%1009, 'hello' FROM
> > generate_series(1,100) s(i)); create statistics test_s1 on id1,
> > id2 from test; analyze;
> >
> > =# EXPLAIN (STATS) SELECT * FROM test WHERE id1 = 1 AND (id2 = 2 OR id2 >
> > 10);
> > QUERY PLAN
> > --
> > - Gather (cost=1000.00..23092.77 rows=84311
> > width=20)
> >Workers Planned: 2
> >-> Parallel Seq Scan on test (cost=0.00..13661.67 rows=35130 width=20)
> > Filter: ((id1 = 1) AND ((id2 = 2) OR (id2 > 10)))
> -- here
> > Ext Stats: public.test_s1 Clauses: (((id2 = 2) OR (id2 > 10)) AND
> > (id1 = 1))
> -- here
> > (5 rows)
> >
>
> I don't think we need to make the order consistent. It probably wouldn't
> hurt, but I'm
> not sure it's even possible for all scan types - for example in an index
> scan, the clauses
> might be split between index conditions and filters, etc.
OK, I understand it isn't unexpected behavior.
> > (3)
> >
> > I might be misunderstanding something, but do we need the clauses? Is
> > there any case where users would want to know the clauses? For
> > example, wouldn't the following be sufficient?
> >
> >> Ext Stats: id1, id2 using test_s1
> >
>
> The stats may overlap, and some clauses may be matching multiple of them. And
> some
> statistics do not support all clause types (e.g.
> functional dependencies work only with equality conditions). Yes, you might
> deduce
> which statistics are used for which clause, but it's not trivial -
> interpreting explain is
> already not trivial, let's not make it harder.
>
> (If tracking the exact clauses turns out to be expensive, we might revisit
> this - it might
> make it cheaper).
Thanks. I agree that we need to show the clauses.
> > (4)
> >
> > The extended statistics with "dependencies" or "ndistinct" option
> > don't seem to be shown in EXPLAIN output. Am I missing something? (Is
> > this expected?)
> >
> > I tested the examples in the documentation. Although it might work
> > with "mcv" option, I can't confirm that it works because "unrecognized node
> > type"
> > error occurred in my environment.
> > https://urldefense.com/v3/__https://www.postgresql.org/docs/current/sq
> > l-createstatistics.html__;!!GCTRfqYYOYGmgK_z!9H-FTXrhg7cr0U2r4PoKEeWM1
> > v9feP8I8zlNyhf-801n-KI8bIMAxOQgaetSTpek3ECk2_FKWEsuApVZ-ys-ka7rfjX8ANB
> > 9zQ$
> >
> > (It might be wrong since I'm beginner with extended stats codes.)
> > IIUC, the reason is that the patch only handles
> > statext_mcv_clauselist_selectivity(),
> > and doesn't handle dependencies_clauselist_selectivity() and
> estimate_multivariate_ndistinct().
> >
> >
> > -- doesn't work with "dependencies" option?
> > =# \dX
> > List of extended statistics
> > Schema | Name | Definition | Ndistinct | Dependencies | MCV
> > +-++---+--+---
> > +-++---+--+---
> > +-++---+--+---
> > public | s1 | a, b FROM t1 | (null)| defined | (null)
> > (2 rows)
> >
> > =# EXPLAIN (STATS, ANALYZE) SELECT * FROM t1 WHERE (a = 1) AND (b = 0);
> > QUERY PLAN
> > --
> > -
> > Gather (cost=1000.00..11685.00 rows=100 width=8) (actual
> time=0.214..50.327 rows=100 loops=1)
> >Workers Planned: 2
> >Workers Launched: 2
> >-> Parallel Seq Scan on t1 (cost=0.00..10675.00 rows=42 width=8)
> > (actual
> time=30.300..46.610 rows=33 loops=3)
> > Filter: ((a = 1) AND (b = 0))
> > Rows Removed by Filter: 00 Planning Time: 0.246 ms
> > Execution Time: 50.361 ms
> > (8 rows)
> >
> > -- doesn't work with "ndistinct"?
> > =# \dX
> > List of extended statistics
> > Schema | Name |Definition
> > |
> Ndistinct | Dependencies | MCV
> >
> +--+--+--
> -+--+
> > public | s3 | date_trunc('month'::text, a), date_trunc('day'::text, a)
> > FROM
Re: Showing applied extended statistics in explain Part 2
Hi, Let me share my opinion on those questions ... On 7/12/24 12:09, masahiro.ik...@nttdata.com wrote: > Hi, > > Thanks for working the feature. As a user, I find it useful, and I'd like to > use > it in v18! Although I've just started start looking into it, I have a few > questions. > > > (1) > > Is it better to make the order of output consistent? For example, even > though there are three clauses shown in the below case, the order does not > match. > * "Filter" shows that "id1" is first. > * "Ext Stats" shows that "id2" is first. > > -- An example > DROP TABLE IF EXISTS test; > CREATE TABLE test (id1 int2, id2 int4, id3 int8, value varchar(32)); > INSERT INTO test (SELECT i%11, i%103, i%1009, 'hello' FROM > generate_series(1,100) s(i)); > create statistics test_s1 on id1, id2 from test; analyze; > > =# EXPLAIN (STATS) SELECT * FROM test WHERE id1 = 1 AND (id2 = 2 OR id2 > 10); > QUERY PLAN > > --- > Gather (cost=1000.00..23092.77 rows=84311 width=20) >Workers Planned: 2 >-> Parallel Seq Scan on test (cost=0.00..13661.67 rows=35130 width=20) > Filter: ((id1 = 1) AND ((id2 = 2) OR (id2 > 10))) > -- here > Ext Stats: public.test_s1 Clauses: (((id2 = 2) OR (id2 > 10)) AND > (id1 = 1))-- here > (5 rows) > I don't think we need to make the order consistent. It probably wouldn't hurt, but I'm not sure it's even possible for all scan types - for example in an index scan, the clauses might be split between index conditions and filters, etc. > > > (2) > > Do we really need the schema names without VERBOSE option? As in the above > case, > "Ext Stats" shows schema name "public", even though the table name "test" > isn't > shown with its schema name. > > Additionally, if the VERBOSE option is specified, should the column names > also be > printed with namespace? > > =# EXPLAIN (VERBOSE, STATS) SELECT * FROM test WHERE id1 = 1 AND (id2 = 2 OR > id2 > 10); > QUERY PLAN > > --- > Gather (cost=1000.00..22947.37 rows=82857 width=20) >Output: id1, id2, id3, value >Workers Planned: 2 >-> Parallel Seq Scan on public.test (cost=0.00..13661.67 rows=34524 > width=20) > Output: id1, id2, id3, value > Filter: ((test.id1 = 1) AND ((test.id2 = 2) OR (test.id2 > 10))) > Ext Stats: public.test_s1 Clauses: (((id2 = 2) OR (id2 > 10)) AND > (id1 = 1)) -- here > (7 rows) > Yeah, I don't think there's a good reason to force printing schema for the statistics, if it's not needed for the table. The rules should be the same, I think. > > > (3) > > I might be misunderstanding something, but do we need the clauses? Is there > any > case where users would want to know the clauses? For example, wouldn't the > following be sufficient? > >> Ext Stats: id1, id2 using test_s1 > The stats may overlap, and some clauses may be matching multiple of them. And some statistics do not support all clause types (e.g. functional dependencies work only with equality conditions). Yes, you might deduce which statistics are used for which clause, but it's not trivial - interpreting explain is already not trivial, let's not make it harder. (If tracking the exact clauses turns out to be expensive, we might revisit this - it might make it cheaper). > > > (4) > > The extended statistics with "dependencies" or "ndistinct" option don't seem > to > be shown in EXPLAIN output. Am I missing something? (Is this expected?) > > I tested the examples in the documentation. Although it might work with > "mcv" option, I can't confirm that it works because "unrecognized node type" > error occurred in my environment. > https://www.postgresql.org/docs/current/sql-createstatistics.html > > (It might be wrong since I'm beginner with extended stats codes.) > IIUC, the reason is that the patch only handles > statext_mcv_clauselist_selectivity(), > and doesn't handle dependencies_clauselist_selectivity() and > estimate_multivariate_ndistinct(). > > > -- doesn't work with "dependencies" option? > =# \dX > List of extended statistics > Schema | Name | Definition | Ndistinct | Dependencies | MCV > +-++---+--+- > public | s1 | a, b FROM t1 | (null)| defined | (null) > (2 rows) > > =# EXPLAIN (STATS, ANALYZE) SELECT * FROM t1 WHERE (a = 1) AND (b = 0); > QUERY PLAN > > --
Re: Showing applied extended statistics in explain Part 2
On 6/28/24 13:16, Tatsuro Yamada wrote: > Hi Tomas and All, > > Attached file is a new patch including: > 6) Add stats option to explain command > 7) The patch really needs some docs (partly) > > >4) Add new node (resolve errors in cfbot and prepared statement) > > I tried adding a new node in pathnode.h, but it doesn't work well. > So, it needs more time to implement it successfully because this is > the first time to add a new node in it. > I'm not sure why it didn't work well, and I haven't tried adding the struct myself so I might be missing something important, but m assumption was the new struct would go to plannodes.h. The planning works in phases: parse -> build Path nodes -> pick cheapest Path -> create Plan and it's the Plan that is printed by EXPLAIN. The pathnodes.h and plannodes.h match this, so if it's expected to be in Plan it should go to plannodes.h I think. > >> 8) Add regression test (stats_ext.sql) > > > Actually, I am not yet able to add new test cases to stats_ext.sql. Why is that not possible? Can you explain? > Instead, I created a simple test (test.sql) and have attached it. > Also, output.txt is the test result. > > To add new test cases to stats_ext.sql, > I'd like to decide on a strategy for modifying it. In particular, there are > 381 places where the check_estimated_rows function is used, so should I > include the same number of tests, or should we include the bare minimum > of tests that cover the code path? I think only the latter would be fine. > Any advice is appreciated. :-D > I don't understand. My suggestion was to create a new function, similar to check_estimated_rows(), that's get a query, do EXPLAIN and extract the list of applied statistics. Similar to what check_estimated_rows() does for number of rows. I did not mean to suggest you modify check_estimated_rows() to extract both the number of rows and statistics, nor to modify the existing tests (that's not very useful, because there's only one extended statistics in each of those tests, and by testing the estimate we implicitly test that it's applied). My suggestion is to add a couple new queries, with multiple statistics and multiple clauses etc. And then test the patch on those. You could do simple EXPLAIN (COSTS OFF), or add the new function to make it a bit more stable (but maybe it's not worth it). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Showing applied extended statistics in explain Part 2
On 6/26/24 11:06, Tatsuro Yamada wrote:
> Hi Tomas!
>
> Thanks for the comments!
>
> 1) The patch is not added to the CF app, which I think is a mistake. Can
>> you please add it to the 2024-07 commitfest? Otherwise people may not be
>> aware of it, won't do reviews etc. It'll require posting a rebased
>> patch, but should not be a big deal.
>>
>
> I added the patch to the 2024-07 commitfest today.
>
>
> 2) Not having the patch in a CF also means cfbot is not running tests on
>> it. Which is unfortunate, because the patch actually has an a bug cfbot
>> would find - I've noticed it after running the tests through the github
>> CI, see [2].
>> 3) The bug has this symptom:
>> ERROR: unrecognized node type: 268
>> CONTEXT: PL/pgSQL function check_estimated_rows(text) line 7 ...
>> STATEMENT: SELECT * FROM check_estimated_rows('SELECT a, b FROM ...
>> 4) I can think of two basic ways to fix this issue - either allow
>> copying of the StatisticExtInto node, or represent the information in a
>> different way (e.g. add a new node for that purpose, or use existing
>> nodes to do that).
>>
>
> Thanks for the info. I'll investigate using cfbot.
> To fix the problem, I understand we need to create a new struct like
> (statistics OID, list of clauses, flag is_or).
>
Yes, something like that, in the plannodes.h.
>
> 5) In [3] Tom raised two possible issues with doing this - cost of
>> copying the information, and locking. For the performance concerns, I
>> think the first thing we should do is measuring how expensive it is. I
>> suggest measuring the overhead for about three basic cases:
>>
>
> Okay, I'll measure it once the patch is completed and check the overhead.
> I read [3][4] and in my opinion I agree with Robert.
> As with indexes, there should be a mechanism for determining whether
> extended statistics are used or not. If it were available, users would be
> able to
> tune using extended statistics and get better execution plans.
>
I do agree with that, but I also understand Tom's concerns about the
costs. His concern is that to make this work, we have to keep/copy the
information for all queries, even if that user never does explain.
Yes, we do the same thing (copy of some pieces) for indexes, and from
this point of view it's equally reasonable. But there's the difference
that for indexes it's always been done this way, hence it's considered
"the baseline", while for extended stats we've not copied the data until
this patch, so it'd be seen as a regression.
I think there are two ways to deal with this - ideally, we'd show that
the overhead is negligible (~noise). And if it's measurable, we'd need
to argue that it's worth it - but that's much harder, IMHO.
So I'd suggest you try to measure the overhead on a couple cases (simple
query with 0 or more statistics applied).
>
>
>> 6) I'm not sure we want to have this under EXPLAIN (VERBOSE). It's what
>> I did in the initial PoC patch, but maybe we should invent a new flag
>> for this purpose, otherwise VERBOSE will cover too much stuff? I'm
>> thinking about "STATS" for example.
>>
>> This would probably mean the patch should also add a new auto_explain
>> "log_" flag to enable/disable this.
>>
>
> I thought it might be better to do this, so I'll fix it.
>
OK
>
>
>> 7) The patch really needs some docs - I'd mention this in the EXPLAIN
>> docs, probably. There's also a chapter about estimates, maybe that
>> should mention this too? Try searching for places in the SGML docs
>> mentioning extended stats and/or explain, I guess.
>>
>
> I plan to create documentation after the specifications are finalized.
>
I'm, not sure that's a good approach. Maybe it doesn't need to be
mentioned in the section explaining how estimates work, but it'd be good
to have it at least in the EXPLAIN command docs. The thing is - docs are
a nice way for reviewers to learn about how the feature is expected to
work / be used. Yes, it may need to be adjusted if the patch changes,
but it's likely much easier than changing the code.
>
>
>> For tests, I guess stats_ext is the right place to test this. I'm not
>> sure what's the best way to do this, though. If it's covered by VERBOSE,
>> that seems it might be unstable - and that would be an issue. But maybe
>> we might add a function similar to check_estimated_rows(), but verifying
>> the query used the expected statistics to estimate expected clauses.
>>
>
> As for testing, I think it's more convenient for reviewers to include it in
> the patch,
> so I'm thinking of including it in the next patch.
>
I'm not sure I understand what you mean - what is more convenient to
include in the patch & you plan to include in the next patch version?
My opinion is that there clearly need to be some regression tests, be it
in stats_ext.sql or in some other script. But to make it easier, we
might have a function similar to check_estimated_rows() which would
extract just the interesting part of the plan.
>
> So ther
RE: Showing applied extended statistics in explain Part 2
Hi,
Thanks for working the feature. As a user, I find it useful, and I'd like to use
it in v18! Although I've just started start looking into it, I have a few
questions.
(1)
Is it better to make the order of output consistent? For example, even
though there are three clauses shown in the below case, the order does not
match.
* "Filter" shows that "id1" is first.
* "Ext Stats" shows that "id2" is first.
-- An example
DROP TABLE IF EXISTS test;
CREATE TABLE test (id1 int2, id2 int4, id3 int8, value varchar(32));
INSERT INTO test (SELECT i%11, i%103, i%1009, 'hello' FROM
generate_series(1,100) s(i));
create statistics test_s1 on id1, id2 from test; analyze;
=# EXPLAIN (STATS) SELECT * FROM test WHERE id1 = 1 AND (id2 = 2 OR id2 > 10);
QUERY PLAN
---
Gather (cost=1000.00..23092.77 rows=84311 width=20)
Workers Planned: 2
-> Parallel Seq Scan on test (cost=0.00..13661.67 rows=35130 width=20)
Filter: ((id1 = 1) AND ((id2 = 2) OR (id2 > 10)))
-- here
Ext Stats: public.test_s1 Clauses: (((id2 = 2) OR (id2 > 10)) AND
(id1 = 1))-- here
(5 rows)
(2)
Do we really need the schema names without VERBOSE option? As in the above case,
"Ext Stats" shows schema name "public", even though the table name "test" isn't
shown with its schema name.
Additionally, if the VERBOSE option is specified, should the column names also
be
printed with namespace?
=# EXPLAIN (VERBOSE, STATS) SELECT * FROM test WHERE id1 = 1 AND (id2 = 2 OR
id2 > 10);
QUERY PLAN
---
Gather (cost=1000.00..22947.37 rows=82857 width=20)
Output: id1, id2, id3, value
Workers Planned: 2
-> Parallel Seq Scan on public.test (cost=0.00..13661.67 rows=34524
width=20)
Output: id1, id2, id3, value
Filter: ((test.id1 = 1) AND ((test.id2 = 2) OR (test.id2 > 10)))
Ext Stats: public.test_s1 Clauses: (((id2 = 2) OR (id2 > 10)) AND
(id1 = 1)) -- here
(7 rows)
(3)
I might be misunderstanding something, but do we need the clauses? Is there any
case where users would want to know the clauses? For example, wouldn't the
following be sufficient?
> Ext Stats: id1, id2 using test_s1
(4)
The extended statistics with "dependencies" or "ndistinct" option don't seem to
be shown in EXPLAIN output. Am I missing something? (Is this expected?)
I tested the examples in the documentation. Although it might work with
"mcv" option, I can't confirm that it works because "unrecognized node type"
error occurred in my environment.
https://www.postgresql.org/docs/current/sql-createstatistics.html
(It might be wrong since I'm beginner with extended stats codes.)
IIUC, the reason is that the patch only handles
statext_mcv_clauselist_selectivity(),
and doesn't handle dependencies_clauselist_selectivity() and
estimate_multivariate_ndistinct().
-- doesn't work with "dependencies" option?
=# \dX
List of extended statistics
Schema | Name | Definition | Ndistinct | Dependencies | MCV
+-++---+--+-
public | s1 | a, b FROM t1 | (null)| defined | (null)
(2 rows)
=# EXPLAIN (STATS, ANALYZE) SELECT * FROM t1 WHERE (a = 1) AND (b = 0);
QUERY PLAN
---
Gather (cost=1000.00..11685.00 rows=100 width=8) (actual time=0.214..50.327
rows=100 loops=1)
Workers Planned: 2
Workers Launched: 2
-> Parallel Seq Scan on t1 (cost=0.00..10675.00 rows=42 width=8) (actual
time=30.300..46.610 rows=33 loops=3)
Filter: ((a = 1) AND (b = 0))
Rows Removed by Filter: 00
Planning Time: 0.246 ms
Execution Time: 50.361 ms
(8 rows)
-- doesn't work with "ndistinct"?
=# \dX
List of extended statistics
Schema | Name |Definition
| Ndistinct | Dependencies | MCV
+--+--+---+--+
public | s3 | date_trunc('month'::text, a), date_trunc('day'::text, a) FROM
t3 | defined | (null) | (null)
(1 row)
postgres(437635)=# EXPLAIN (STATS, ANALYZE) SELECT * FROM t3
WHERE date_trunc('month', a) = '2020-01-01'::timestamp;
QUERY PLAN
Re: Showing applied extended statistics in explain Part 2
Hi Tomas and All, Attached file is a new patch including: 6) Add stats option to explain command 7) The patch really needs some docs (partly) >4) Add new node (resolve errors in cfbot and prepared statement) I tried adding a new node in pathnode.h, but it doesn't work well. So, it needs more time to implement it successfully because this is the first time to add a new node in it. > 8) Add regression test (stats_ext.sql) Actually, I am not yet able to add new test cases to stats_ext.sql. Instead, I created a simple test (test.sql) and have attached it. Also, output.txt is the test result. To add new test cases to stats_ext.sql, I'd like to decide on a strategy for modifying it. In particular, there are 381 places where the check_estimated_rows function is used, so should I include the same number of tests, or should we include the bare minimum of tests that cover the code path? I think only the latter would be fine. Any advice is appreciated. :-D P.S. I'm going to investigate how to use CI this weekend hopefully. Regards, Tatsuro Yamada CREATE FUNCTION CREATE FUNCTION CREATE TABLE INSERT 0 10 QUERY PLAN --- Seq Scan on t (cost=0.00..1944.77 rows=3 width=8) (actual time=0.049..37.182 rows=1 loops=1) Filter: ((a = 1) AND (b = 1)) Rows Removed by Filter: 9 Planning Time: 0.380 ms Execution Time: 37.751 ms (5 rows) clause | extstats +-- (0 rows) QUERY PLAN --- HashAggregate (cost=1944.77..2044.89 rows=10012 width=12) Group Key: a, b -> Seq Scan on t (cost=0.00..1444.18 rows=100118 width=8) (3 rows) clause | extstats +-- (0 rows) CREATE STATISTICS ANALYZE List of extended statistics Schema | Name | Definition | Ndistinct | Dependencies | MCV +--+-+---+--+- public | s| a, b FROM t | defined | defined | defined (1 row) QUERY PLAN -- Seq Scan on t (cost=0.00..1943.00 rows=9947 width=8) (actual time=0.021..26.203 rows=1 loops=1) Filter: ((a = 1) AND (b = 1)) Rows Removed by Filter: 9 Ext Stats: public.s Clauses: ((a = 1) AND (b = 1)) Planning Time: 0.169 ms Execution Time: 26.958 ms (6 rows) clause | extstats ---+-- Filter: ((a = 1) AND (b = 1)) | public.s Clauses: ((a = 1) AND (b = 1)) (1 row) QUERY PLAN --- HashAggregate (cost=1943.00..1943.10 rows=10 width=12) (actual time=96.912..96.918 rows=10 loops=1) Group Key: a, b Batches: 1 Memory Usage: 24kB -> Seq Scan on t (cost=0.00..1443.00 rows=10 width=8) (actual time=0.026..24.280 rows=10 loops=1) Ext Stats: public.s Clauses: a, b Planning Time: 0.353 ms Execution Time: 96.974 ms (7 rows) clause |extstats -+- Group Key: a, b | public.s Clauses: a, b (1 row) SET QUERY PLAN --- HashAggregate (cost=1943.00..1943.10 rows=10 width=12) (actual time=89.360..89.366 rows=10 loops=1) Group Key: a, b Batches: 1 Memory Usage: 24kB -> Seq Scan on t (cost=0.00..1443.00 rows=10 width=8) (actual time=0.017..19.903 rows=10 loops=1) Ext Stats: public.s Clauses: a, b Ext Stats: public.s Clauses: a, b Planning Time: 0.094 ms Execution Time: 89.409 ms (8 rows) clause |extstats -+- Group Key: a, b | public.s Clauses: a, b (1 row) PREPARE QUERY PLAN -- Seq Scan on t (cost=0.00..1943.00 rows=9850 width=8) (actual time=0.024..27.818 rows=1 loops=1) Filter: ((a = 5) AND (b = 5)) Rows Removed by Filter: 9 Planning Time: 0.108 ms Execution Time: 28.510 ms (5 rows) DROP TABLE DROP FUNCTION List of extended statistics Schema
Re: Showing applied extended statistics in explain Part 2
Hi Tomas, The attached patch does not correspond to the above comment. > But it does solve some of the issues mentioned in previous threads. > Oops, I made a mistake sending a patch on my previous email. Attached patch is the right patch. Regards, Tatsuro Yamada 0001-Add-Showing-applied-extended-statistics-in-explain-r3.patch Description: Binary data
Re: Showing applied extended statistics in explain Part 2
Hi Tomas!
Thanks for the comments!
1) The patch is not added to the CF app, which I think is a mistake. Can
> you please add it to the 2024-07 commitfest? Otherwise people may not be
> aware of it, won't do reviews etc. It'll require posting a rebased
> patch, but should not be a big deal.
>
I added the patch to the 2024-07 commitfest today.
2) Not having the patch in a CF also means cfbot is not running tests on
> it. Which is unfortunate, because the patch actually has an a bug cfbot
> would find - I've noticed it after running the tests through the github
> CI, see [2].
> 3) The bug has this symptom:
> ERROR: unrecognized node type: 268
> CONTEXT: PL/pgSQL function check_estimated_rows(text) line 7 ...
> STATEMENT: SELECT * FROM check_estimated_rows('SELECT a, b FROM ...
> 4) I can think of two basic ways to fix this issue - either allow
> copying of the StatisticExtInto node, or represent the information in a
> different way (e.g. add a new node for that purpose, or use existing
> nodes to do that).
>
Thanks for the info. I'll investigate using cfbot.
To fix the problem, I understand we need to create a new struct like
(statistics OID, list of clauses, flag is_or).
5) In [3] Tom raised two possible issues with doing this - cost of
> copying the information, and locking. For the performance concerns, I
> think the first thing we should do is measuring how expensive it is. I
> suggest measuring the overhead for about three basic cases:
>
Okay, I'll measure it once the patch is completed and check the overhead.
I read [3][4] and in my opinion I agree with Robert.
As with indexes, there should be a mechanism for determining whether
extended statistics are used or not. If it were available, users would be
able to
tune using extended statistics and get better execution plans.
> 6) I'm not sure we want to have this under EXPLAIN (VERBOSE). It's what
> I did in the initial PoC patch, but maybe we should invent a new flag
> for this purpose, otherwise VERBOSE will cover too much stuff? I'm
> thinking about "STATS" for example.
>
> This would probably mean the patch should also add a new auto_explain
> "log_" flag to enable/disable this.
>
I thought it might be better to do this, so I'll fix it.
> 7) The patch really needs some docs - I'd mention this in the EXPLAIN
> docs, probably. There's also a chapter about estimates, maybe that
> should mention this too? Try searching for places in the SGML docs
> mentioning extended stats and/or explain, I guess.
>
I plan to create documentation after the specifications are finalized.
> For tests, I guess stats_ext is the right place to test this. I'm not
> sure what's the best way to do this, though. If it's covered by VERBOSE,
> that seems it might be unstable - and that would be an issue. But maybe
> we might add a function similar to check_estimated_rows(), but verifying
> the query used the expected statistics to estimate expected clauses.
>
As for testing, I think it's more convenient for reviewers to include it in
the patch,
so I'm thinking of including it in the next patch.
So there's stuff to do to make this committable, but hopefully this
> review gives you some guidance regarding what/how ;-)
>
Thank you! It helps me a lot!
The attached patch does not correspond to the above comment.
But it does solve some of the issues mentioned in previous threads.
The next patch is planned to include:
6) Add stats option to explain command
8) Add regression test (stats_ext.sql)
4) Add new node (resolve errors in cfbot and prepared statement)
Regards,
Tatsuro Yamada
> [1]
>
> https://www.postgresql.org/message-id/TYYPR01MB82310B308BA8770838F681619E5E2%40TYYPR01MB8231.jpnprd01.prod.outlook.com
>
> [2] https://cirrus-ci.com/build/6436352672137216
>
> [3]
> https://www.postgresql.org/message-id/459863.1627419001%40sss.pgh.pa.us
>
> [4]
>
> https://www.postgresql.org/message-id/CA%2BTgmoZU34zo4%3DhyqgLH16iGpHQ6%2BQAesp7k5a1cfZB%3D%2B9xtsw%40mail.gmail.com
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
>
0001-show-stats-in-explain-rebased-on-15c9ac36.patch
Description: Binary data
Re: Showing applied extended statistics in explain Part 2
Hello Yamada-san,
I finally got to look at this patch again - apologies for taking so
long, I'm well aware it's rather frustrating to wait for feedback. I'll
try to pay more attention to this patch, and don't hesitate to ping me
off-list if immediate input is needed.
I looked at the patch from March 1 [1], which applies mostly with some
minor bitrot, but no major conflicts. A couple review comments:
1) The patch is not added to the CF app, which I think is a mistake. Can
you please add it to the 2024-07 commitfest? Otherwise people may not be
aware of it, won't do reviews etc. It'll require posting a rebased
patch, but should not be a big deal.
2) Not having the patch in a CF also means cfbot is not running tests on
it. Which is unfortunate, because the patch actually has an a bug cfbot
would find - I've noticed it after running the tests through the github
CI, see [2].
FWIW I very much recommend setting up this CI and using it during
development, it turned out to be very valuable for me as it tests on a
range of systems, and more convenient than the rpi5 machines I used for
that purposes before. See src/tools/ci/README for details.
3) The bug has this symptom:
ERROR: unrecognized node type: 268
CONTEXT: PL/pgSQL function check_estimated_rows(text) line 7 ...
STATEMENT: SELECT * FROM check_estimated_rows('SELECT a, b FROM ...
but it only shows on the FreeBSD machine (in CI). But that's simply
because that's running tests with "-DCOPY_PARSE_PLAN_TREES", which
always copies the query plan, to make sure all the nodes can be copied.
And we don't have a copy function for the StatisticExtInfo node (that's
the node 268), so it fails.
FWIW you can have this failure even on a regular build, you just need to
do explain on a prepared statement (with an extended statistics):
CREATE TABLE t (a int, b int);
INSERT INTO t SELECT (i/100), (i/100)
FROM generate_series(1,1000) s(i);
CREATE STATISTICS ON a,b FROM t;
ANALYZE t;
PREPARE ps (INT, INT) AS SELECT * FROM t WHERE a = $1 AND b = $2;
EXPLAIN EXECUTE ps(5,5);
ERROR: unrecognized node type: 268
4) I can think of two basic ways to fix this issue - either allow
copying of the StatisticExtInto node, or represent the information in a
different way (e.g. add a new node for that purpose, or use existing
nodes to do that).
I don't think we should do the first thing - the StatisticExtInfo is in
pathnodes.h, is designed to be used during path construction, has a lot
of fields that we likely don't need to show stuff in explain - but we'd
need to copy those too, and that seems useless / expensive.
So I suggest we invent a new (much simpler) node, tracking only the bits
we actually need for in the explain part. Or alternatively, if you think
adding a separate node is an overkill, maybe we could keep just an OID
of the statistics we applied, and the explain would lookup the name?
But I think having a new node might might also make the patch simpler,
as the new struct could combine the information the patch keeps in three
separate lists. Instead, there's be just one new list in Plan, members
would be the new node type, and each element would be
(statistics OID, list of clauses, flag is_or)
or something like that.
5) In [3] Tom raised two possible issues with doing this - cost of
copying the information, and locking. For the performance concerns, I
think the first thing we should do is measuring how expensive it is. I
suggest measuring the overhead for about three basic cases:
- table with no extended stats
- table with a couple (1-10?) extended stats
- table with a lot of (100?) extended stats
And see the impact of the patch. That is, measure the planning time with
master and with the patch applied. The table size does not matter much,
I think - this should measure just the planning, not execute the query.
In practice the extra costs will get negligible as the execution time
grows. But we're measuring the worst case, so that's fine.
For the locking, I agree with Robert [4] that this probably should not
be an issue - I don't see why would this be different from indexes etc.
But I haven't thought about that too much, so maybe investigate and test
this a bit more (that plans get invalidated if the statistics changes,
and so on).
6) I'm not sure we want to have this under EXPLAIN (VERBOSE). It's what
I did in the initial PoC patch, but maybe we should invent a new flag
for this purpose, otherwise VERBOSE will cover too much stuff? I'm
thinking about "STATS" for example.
This would probably mean the patch should also add a new auto_explain
"log_" flag to enable/disable this.
7) The patch really needs some docs - I'd mention this in the EXPLAIN
docs, probably. There's also a chapter about estimates, maybe that
should mention this too? Try searching for places in the SGML docs
mentioning extended stats and/or explain, I guess.
For tests, I guess stats_ext is the right place to test this. I'm not
sure what's the best way
Re: Showing applied extended statistics in explain Part 2
On 3/1/24 01:19, Tatsuro Yamada wrote: > Hi, > > This original patch made by Tomas improves the usability of extended > statistics, > so I rebased it on 362de947, and I'd like to re-start developing it. > > The previous thread [1] suggested something to solve. I'll try to solve it as > best I can, but I think this feature is worth it with some limitations. > Please find the attached file. > Thank you for the interest in moving this patch forward. And I agree it's worth to cut some of the stuff if it's necessary to make it work. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
