Обсуждение: improve transparency of bitmap-only heap scans
When bitmap-only heap scans were introduced in v11 (7c70996ebf0949b142a99) no changes were made to "EXPLAIN". This makes the feature rather opaque. You can sometimes figure out what is going by the output of EXPLAIN (ANALYZE, BUFFERS), but that is unintuitive and fragile.
Looking at the discussion where the feature was added, I think changing the EXPLAIN just wasn't considered.
The attached patch adds "avoided" to "exact" and "lossy" as a category under
"Heap Blocks". Also attached is the example output, as the below will probably wrap to the point of illegibility:
"Heap Blocks". Also attached is the example output, as the below will probably wrap to the point of illegibility:
explain analyze select count(*) from foo where a=35 and d between 67 and 70;
QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------------------------
Aggregate (cost=21451.36..21451.37 rows=1 width=8) (actual time=103.955..103.955 rows=1 loops=1)
-> Bitmap Heap Scan on foo (cost=9920.73..21442.44 rows=3570 width=0) (actual time=100.239..103.204 rows=3950 loops=1)
Recheck Cond: ((a = 35) AND (d >= 67) AND (d <= 70))
Heap Blocks: avoided=3718 exact=73
-> BitmapAnd (cost=9920.73..9920.73 rows=3570 width=0) (actual time=98.666..98.666 rows=0 loops=1)
-> Bitmap Index Scan on foo_a_c_idx (cost=0.00..1682.93 rows=91000 width=0) (actual time=28.541..28.541 rows=99776 loops=1)
Index Cond: (a = 35)
-> Bitmap Index Scan on foo_d_idx (cost=0.00..8235.76 rows=392333 width=0) (actual time=66.946..66.946 rows=399003 loops=1)
Index Cond: ((d >= 67) AND (d <= 70))
Planning Time: 0.458 ms
Execution Time: 104.487 ms
I think the name of the node should also be changed to "Bitmap Only Heap Scan", but I didn't implement that as adding another NodeTag looks like a lot of tedious error prone work to do before getting feedback on whether the change is desirable in the first place, or the correct approach.
Cheers,
Jeff
Вложения
> Looking at the discussion where the feature was added, I think changing the > EXPLAIN just wasn't considered. I think this is an oversight. It is very useful to have this on EXPLAIN. > The attached patch adds "avoided" to "exact" and "lossy" as a category > under "Heap Blocks". It took me a while to figure out what those names mean. "unfetched", as you call it on the code, may be more descriptive than "avoided" for the new label. However I think the other two are more confusing. It may be a good idea to change them together with this. > I think the name of the node should also be changed to "Bitmap Only Heap > Scan", but I didn't implement that as adding another NodeTag looks like a > lot of tedious error prone work to do before getting feedback on whether > the change is desirable in the first place, or the correct approach. I am not sure about this part. In my opinion it may have been easier to explain to users if "Index Only Scan" had not been separate but "Index Scan" optimization.
Hello, > It took me a while to figure out what those names mean. "unfetched", > as you call it on the code, may be more descriptive than "avoided" for > the new label. However I think the other two are more confusing. It > may be a good idea to change them together with this. It'll be sad if this patch is forgotten only because of the words choice. I've changed it all to "unfetched" for at least not to call the same thing differently in the code and in the output, and also rebased it and fit in 80 lines width limit. Best, Alex
Вложения
On Fri, Feb 07, 2020 at 03:22:12PM +0000, Alexey Bashtanov wrote: >Hello, > >>It took me a while to figure out what those names mean. "unfetched", >>as you call it on the code, may be more descriptive than "avoided" for >>the new label. However I think the other two are more confusing. It >>may be a good idea to change them together with this. >It'll be sad if this patch is forgotten only because of the words choice. >I've changed it all to "unfetched" for at least not to call the same >thing differently >in the code and in the output, and also rebased it and fit in 80 lines >width limit. > I kinda suspect one of the ressons why this got so little attention is that it was never added to any CF. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> I kinda suspect one of the ressons why this got so little attention is > that it was never added to any CF. Thanks Tomas, I've created a CF entry https://commitfest.postgresql.org/27/2443/ Best, Alex
Hi Jeff, On 2/7/20 10:22 AM, Alexey Bashtanov wrote: > I've changed it all to "unfetched" for at least not to call the same > thing differently > in the code and in the output, and also rebased it and fit in 80 lines > width limit. What do you think of Alexey's updates? Regards, -- -David david@pgmasters.net
On Tue, Mar 10, 2020 at 12:15 PM David Steele <david@pgmasters.net> wrote: > > Hi Jeff, > > On 2/7/20 10:22 AM, Alexey Bashtanov wrote: > > I've changed it all to "unfetched" for at least not to call the same > > thing differently > > in the code and in the output, and also rebased it and fit in 80 lines > > width limit. > > What do you think of Alexey's updates? > > Regards, > -- > -David > david@pgmasters.net I've added myself as a reviewer. The patch looks good to me. It doesn't seem to have much risk either; there are not spec concerns applicable (since it's EXPLAIN), and the surface area for impact quite small. Both make check and check-world pass. Here's a test query setup I worked up: create table exp(a int, d int); insert into exp(a, d) select random() * 100, t.i % 50 from generate_series(0,10000000) t(i); create index index_exp_a on exp(a); create index index_exp_d on exp(d); analyze exp; Then: explain analyze select count(*) from exp where a = 25 and d between 5 and 10; shows: Heap Blocks: exact=10518 but if I: vacuum freeze exp; then it shows: Heap Blocks: unfetched=10518 as I'd expect. One question though: if I change the query to: explain (analyze, buffers) select count(*) from exp where a between 50 and 100 and d between 5 and 10; then I get a parallel bitmap heap scan, and I only see exact heap blocks (see attached explain output). Does the original optimization cover parallel bitmap heap scans like this? If not, I think this patch is likely ready for committer. If so, then we still need support for stats tracking and explain output for parallel nodes. I've taken the liberty of: - Reformatting slightly for a cleaner diff. - Running pgindent against the changes - Added a basic commit message. - Add unfetched_pages initialization to ExecInitBitmapHeapScan. See attached. Thanks, James
Вложения
On Mon, Mar 16, 2020 at 9:08 AM James Coleman <jtc331@gmail.com> wrote: > ... > One question though: if I change the query to: > explain (analyze, buffers) select count(*) from exp where a between 50 > and 100 and d between 5 and 10; > then I get a parallel bitmap heap scan, and I only see exact heap > blocks (see attached explain output). > > Does the original optimization cover parallel bitmap heap scans like > this? If not, I think this patch is likely ready for committer. If so, > then we still need support for stats tracking and explain output for > parallel nodes. I've looked at the code a bit more deeply, and the implementation means the optimization applies to parallel scans also. I've also convinced myself that the change in explain.c will cover both non-parallel and parallel plans. Since that's the only question I saw, and the patch seems pretty uncontroversial/not requiring any real design choices, I've gone ahead and marked this patch as ready for committer. Thanks for working on this! James
On Mon, Mar 16, 2020 at 09:08:36AM -0400, James Coleman wrote:
> Does the original optimization cover parallel bitmap heap scans like this?
It works for parallel bitmap only scans.
template1=# explain analyze select count(*) from exp where a between 25 and 35 and d between 5 and 10;
Finalize Aggregate (cost=78391.68..78391.69 rows=1 width=8) (actual time=525.972..525.972 rows=1 loops=1)
-> Gather (cost=78391.47..78391.68 rows=2 width=8) (actual time=525.416..533.133 rows=3 loops=1)
Workers Planned: 2
Workers Launched: 2
-> Partial Aggregate (cost=77391.47..77391.48 rows=1 width=8) (actual time=518.406..518.406 rows=1 loops=3)
-> Parallel Bitmap Heap Scan on exp (cost=31825.37..77245.01 rows=58582 width=0) (actual
time=296.309..508.440rows=43887 loops=3)
Recheck Cond: ((a >= 25) AND (a <= 35) AND (d >= 5) AND (d <= 10))
Heap Blocks: unfetched=4701 exact=9650
-> BitmapAnd (cost=31825.37..31825.37 rows=140597 width=0) (actual time=282.590..282.590 rows=0
loops=1)
-> Bitmap Index Scan on index_exp_a (cost=0.00..15616.99 rows=1166456 width=0) (actual
time=147.036..147.036rows=1099872 loops=1)
Index Cond: ((a >= 25) AND (a <= 35))
-> Bitmap Index Scan on index_exp_d (cost=0.00..16137.82 rows=1205339 width=0) (actual
time=130.366..130.366rows=1200000 loops=1)
Index Cond: ((d >= 5) AND (d <= 10))
> +++ b/src/backend/commands/explain.c
> @@ -2777,6 +2777,8 @@ show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
> {
> if (es->format != EXPLAIN_FORMAT_TEXT)
> {
> + ExplainPropertyInteger("Unfetched Heap Blocks", NULL,
> + planstate->unfetched_pages, es);
> ExplainPropertyInteger("Exact Heap Blocks", NULL,
> planstate->exact_pages, es);
> ExplainPropertyInteger("Lossy Heap Blocks", NULL,
> @@ -2784,10 +2786,14 @@ show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
> }
> else
> {
> - if (planstate->exact_pages > 0 || planstate->lossy_pages > 0)
> + if (planstate->exact_pages > 0 || planstate->lossy_pages > 0
> + || planstate->unfetched_pages > 0)
> {
> ExplainIndentText(es);
> appendStringInfoString(es->str, "Heap Blocks:");
> + if (planstate->unfetched_pages > 0)
> + appendStringInfo(es->str, " unfetched=%ld",
> + planstate->unfetched_pages);
> if (planstate->exact_pages > 0)
> appendStringInfo(es->str, " exact=%ld", planstate->exact_pages);
> if (planstate->lossy_pages > 0)
I don't think it matters in nontext mode, but at least in text mode, I think
maybe the Unfetched blocks should be output after the exact and lossy blocks,
in case someone is parsing it, and because bitmap-only is a relatively new
feature. Its output is probably less common than exact/lossy.
--
Justin
On Thu, Mar 19, 2020 at 9:26 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Mar 16, 2020 at 09:08:36AM -0400, James Coleman wrote:
> > Does the original optimization cover parallel bitmap heap scans like this?
>
> It works for parallel bitmap only scans.
>
> template1=# explain analyze select count(*) from exp where a between 25 and 35 and d between 5 and 10;
> Finalize Aggregate (cost=78391.68..78391.69 rows=1 width=8) (actual time=525.972..525.972 rows=1 loops=1)
> -> Gather (cost=78391.47..78391.68 rows=2 width=8) (actual time=525.416..533.133 rows=3 loops=1)
> Workers Planned: 2
> Workers Launched: 2
> -> Partial Aggregate (cost=77391.47..77391.48 rows=1 width=8) (actual time=518.406..518.406 rows=1
loops=3)
> -> Parallel Bitmap Heap Scan on exp (cost=31825.37..77245.01 rows=58582 width=0) (actual
time=296.309..508.440rows=43887 loops=3)
> Recheck Cond: ((a >= 25) AND (a <= 35) AND (d >= 5) AND (d <= 10))
> Heap Blocks: unfetched=4701 exact=9650
> -> BitmapAnd (cost=31825.37..31825.37 rows=140597 width=0) (actual time=282.590..282.590
rows=0loops=1)
> -> Bitmap Index Scan on index_exp_a (cost=0.00..15616.99 rows=1166456 width=0) (actual
time=147.036..147.036rows=1099872 loops=1)
> Index Cond: ((a >= 25) AND (a <= 35))
> -> Bitmap Index Scan on index_exp_d (cost=0.00..16137.82 rows=1205339 width=0) (actual
time=130.366..130.366rows=1200000 loops=1)
> Index Cond: ((d >= 5) AND (d <= 10))
>
>
> > +++ b/src/backend/commands/explain.c
> > @@ -2777,6 +2777,8 @@ show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
> > {
> > if (es->format != EXPLAIN_FORMAT_TEXT)
> > {
> > + ExplainPropertyInteger("Unfetched Heap Blocks", NULL,
> > + planstate->unfetched_pages, es);
> > ExplainPropertyInteger("Exact Heap Blocks", NULL,
> > planstate->exact_pages, es);
> > ExplainPropertyInteger("Lossy Heap Blocks", NULL,
> > @@ -2784,10 +2786,14 @@ show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
> > }
> > else
> > {
> > - if (planstate->exact_pages > 0 || planstate->lossy_pages > 0)
> > + if (planstate->exact_pages > 0 || planstate->lossy_pages > 0
> > + || planstate->unfetched_pages > 0)
> > {
> > ExplainIndentText(es);
> > appendStringInfoString(es->str, "Heap Blocks:");
> > + if (planstate->unfetched_pages > 0)
> > + appendStringInfo(es->str, " unfetched=%ld",
> > + planstate->unfetched_pages);
> > if (planstate->exact_pages > 0)
> > appendStringInfo(es->str, " exact=%ld", planstate->exact_pages);
> > if (planstate->lossy_pages > 0)
Awesome, thanks for confirming with an actual plan.
> I don't think it matters in nontext mode, but at least in text mode, I think
> maybe the Unfetched blocks should be output after the exact and lossy blocks,
> in case someone is parsing it, and because bitmap-only is a relatively new
> feature. Its output is probably less common than exact/lossy.
I tweaked that (and a comment that didn't reference the change); see attached.
James
Вложения
On Fri, Mar 20, 2020 at 7:09 AM James Coleman <jtc331@gmail.com> wrote: > > Awesome, thanks for confirming with an actual plan. > > > I don't think it matters in nontext mode, but at least in text mode, I think > > maybe the Unfetched blocks should be output after the exact and lossy blocks, > > in case someone is parsing it, and because bitmap-only is a relatively new > > feature. Its output is probably less common than exact/lossy. > > I tweaked that (and a comment that didn't reference the change); see attached. > Few comments: 1. - - if (tbmres->ntuples >= 0) + else if (tbmres->ntuples >= 0) node->exact_pages++; How is this change related to this patch? 2. + * unfetched_pages total number of pages not retrieved due to vm * prefetch_iterator iterator for prefetching ahead of current page * prefetch_pages # pages prefetch iterator is ahead of current * prefetch_target current target prefetch distance @@ -1591,6 +1592,7 @@ typedef struct BitmapHeapScanState Buffer pvmbuffer; long exact_pages; long lossy_pages; + long unfetched_pages; Can we name it as skipped_pages? 3. Can we add a test or two for this functionality? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 24, 2020 at 10:54:05AM +0530, Amit Kapila wrote:
> On Fri, Mar 20, 2020 at 7:09 AM James Coleman <jtc331@gmail.com> wrote:
> >
> > Awesome, thanks for confirming with an actual plan.
> >
> > > I don't think it matters in nontext mode, but at least in text mode, I think
> > > maybe the Unfetched blocks should be output after the exact and lossy blocks,
> > > in case someone is parsing it, and because bitmap-only is a relatively new
> > > feature. Its output is probably less common than exact/lossy.
> >
> > I tweaked that (and a comment that didn't reference the change); see attached.
> >
>
> Few comments:
> 1.
> -
> - if (tbmres->ntuples >= 0)
> + else if (tbmres->ntuples >= 0)
> node->exact_pages++;
>
> How is this change related to this patch?
Previously, a page was either "exact" or "lossy".
Now it's one of exact/lossy/skipped.
(But not exact/lossy but in either case might be skipped).
if (skip_fetch)
{
/* can't be lossy in the skip_fetch case */
Assert(tbmres->ntuples >= 0);
/*
* The number of tuples on this page is put into
* node->return_empty_tuples.
*/
node->return_empty_tuples = tbmres->ntuples;
+ node->unfetched_pages++;
}
else if (!table_scan_bitmap_next_block(scan, tbmres))
{
/* AM doesn't think this block is valid, skip */
continue;
}
-
- if (tbmres->ntuples >= 0)
+ else if (tbmres->ntuples >= 0)
node->exact_pages++;
else
node->lossy_pages++;
--
Justin
On Tue, Mar 24, 2020 at 11:36 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Tue, Mar 24, 2020 at 10:54:05AM +0530, Amit Kapila wrote: > > On Fri, Mar 20, 2020 at 7:09 AM James Coleman <jtc331@gmail.com> wrote: > > > > > > Awesome, thanks for confirming with an actual plan. > > > > > > > I don't think it matters in nontext mode, but at least in text mode, I think > > > > maybe the Unfetched blocks should be output after the exact and lossy blocks, > > > > in case someone is parsing it, and because bitmap-only is a relatively new > > > > feature. Its output is probably less common than exact/lossy. > > > > > > I tweaked that (and a comment that didn't reference the change); see attached. > > > > > > > Few comments: > > 1. > > - > > - if (tbmres->ntuples >= 0) > > + else if (tbmres->ntuples >= 0) > > node->exact_pages++; > > > > How is this change related to this patch? > > Previously, a page was either "exact" or "lossy". > Now it's one of exact/lossy/skipped. > Okay, that makes sense. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 24, 2020 at 1:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Mar 20, 2020 at 7:09 AM James Coleman <jtc331@gmail.com> wrote: > > > > Awesome, thanks for confirming with an actual plan. > > > > > I don't think it matters in nontext mode, but at least in text mode, I think > > > maybe the Unfetched blocks should be output after the exact and lossy blocks, > > > in case someone is parsing it, and because bitmap-only is a relatively new > > > feature. Its output is probably less common than exact/lossy. > > > > I tweaked that (and a comment that didn't reference the change); see attached. > > > > Few comments: > 1. > - > - if (tbmres->ntuples >= 0) > + else if (tbmres->ntuples >= 0) > node->exact_pages++; > > How is this change related to this patch? <already answered by Justin> > 2. > + * unfetched_pages total number of pages not retrieved due to vm > * prefetch_iterator iterator for prefetching ahead of current page > * prefetch_pages # pages prefetch iterator is ahead of current > * prefetch_target current target prefetch distance > @@ -1591,6 +1592,7 @@ typedef struct BitmapHeapScanState > Buffer pvmbuffer; > long exact_pages; > long lossy_pages; > + long unfetched_pages; > > Can we name it as skipped_pages? That seems easy enough to do. > 3. Can we add a test or two for this functionality? From what I can tell the current lossy page count isn't tested either; would we expect the explain output from such a test to be stable across different architectures etc.? James
I took a quick look through this patch. While I see nothing to complain
about implementation-wise, I'm a bit befuddled as to why we need this
reporting when there is no comparable data provided for regular index-only
scans. Over there, you just get "Heap Fetches: n", and the existing
counts for bitmap scans seem to cover the same territory.
I agree with the original comment that it's pretty strange that
EXPLAIN doesn't identify an index-only BMS at all; but fixing that
is a different patch.
regards, tom lane
On Wed, Mar 25, 2020 at 12:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I took a quick look through this patch. While I see nothing to complain
> about implementation-wise, I'm a bit befuddled as to why we need this
> reporting when there is no comparable data provided for regular index-only
> scans. Over there, you just get "Heap Fetches: n", and the existing
> counts for bitmap scans seem to cover the same territory.
>
Isn't deducing similar information ("Skipped Heap Fetches: n") there
is a bit easier than it is here?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 24, 2020 at 11:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 25, 2020 at 12:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > I took a quick look through this patch. While I see nothing to complain
> > about implementation-wise, I'm a bit befuddled as to why we need this
> > reporting when there is no comparable data provided for regular index-only
> > scans. Over there, you just get "Heap Fetches: n", and the existing
> > counts for bitmap scans seem to cover the same territory.
> >
>
> Isn't deducing similar information ("Skipped Heap Fetches: n") there
> is a bit easier than it is here?
While I'm not the patch author so can't speak to the original thought
process, I do think it makes sense to show it. I could imagine a world
in which index only scans were printed in explain as purely an
optimization to index scans that shows exactly this (how many pages we
were able to skip fetching). That approach actually can make things
more helpful than the approach current in explain for index only
scans, since the optimization isn't all or nothing (i.e., it can still
fetch heap pages), so it's interesting to see exactly how much it
gained you.
James
On Wed, Mar 25, 2020 at 5:44 PM James Coleman <jtc331@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 11:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Mar 25, 2020 at 12:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > I took a quick look through this patch. While I see nothing to complain
> > > about implementation-wise, I'm a bit befuddled as to why we need this
> > > reporting when there is no comparable data provided for regular index-only
> > > scans. Over there, you just get "Heap Fetches: n", and the existing
> > > counts for bitmap scans seem to cover the same territory.
> > >
> >
> > Isn't deducing similar information ("Skipped Heap Fetches: n") there
> > is a bit easier than it is here?
>
> While I'm not the patch author so can't speak to the original thought
> process, I do think it makes sense to show it.
>
Yeah, I also see this information could be useful. It seems Tom Lane
is not entirely convinced of this. I am not sure if this is the right
time to seek more opinions as we are already near the end of CF. So,
we should either decide to move this to the next CF if we think of
getting the opinion of others or simply reject it and see a better way
for EXPLAIN to identify an index-only BMS.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Fri, Mar 27, 2020 at 9:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 25, 2020 at 5:44 PM James Coleman <jtc331@gmail.com> wrote:
> >
> > On Tue, Mar 24, 2020 at 11:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Mar 25, 2020 at 12:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > >
> > > > I took a quick look through this patch. While I see nothing to complain
> > > > about implementation-wise, I'm a bit befuddled as to why we need this
> > > > reporting when there is no comparable data provided for regular index-only
> > > > scans. Over there, you just get "Heap Fetches: n", and the existing
> > > > counts for bitmap scans seem to cover the same territory.
> > > >
> > >
> > > Isn't deducing similar information ("Skipped Heap Fetches: n") there
> > > is a bit easier than it is here?
> >
> > While I'm not the patch author so can't speak to the original thought
> > process, I do think it makes sense to show it.
> >
>
> Yeah, I also see this information could be useful. It seems Tom Lane
> is not entirely convinced of this. I am not sure if this is the right
> time to seek more opinions as we are already near the end of CF. So,
> we should either decide to move this to the next CF if we think of
> getting the opinion of others or simply reject it and see a better way
> for EXPLAIN to identify an index-only BMS.
I'm curious if Tom's objection is mostly on the grounds that we should
be consistent in what's displayed, or that he thinks the information
is likely to be useless.
If consistency is the goal you might e.g., do something that just
changes the node type output, but in favor of changing that, it seems
to me that showing "how well did the optimization" is actually more
valuable than "did we do the optimization at all". Additionally I
think showing it as an optimization of an existing node is actually
likely less confusing anyway.
One other thing: my understanding is that this actually matches the
underlying code split too. For the index only scan case, we actually
have a separate node (it's not just an optimization of the standard
index scan). There are discussions about whether that's a good thing,
but it's what we have. In contrast, the bitmap scan actually has it as
a pure optimization of the existing bitmap heap scan node.
James
On Sat, Mar 28, 2020 at 8:02 PM James Coleman <jtc331@gmail.com> wrote: > > On Fri, Mar 27, 2020 at 9:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > Yeah, I also see this information could be useful. It seems Tom Lane > > is not entirely convinced of this. I am not sure if this is the right > > time to seek more opinions as we are already near the end of CF. So, > > we should either decide to move this to the next CF if we think of > > getting the opinion of others or simply reject it and see a better way > > for EXPLAIN to identify an index-only BMS. > > I'm curious if Tom's objection is mostly on the grounds that we should > be consistent in what's displayed, or that he thinks the information > is likely to be useless. > Yeah, it would be good if he clarifies his position. > If consistency is the goal you might e.g., do something that just > changes the node type output, but in favor of changing that, it seems > to me that showing "how well did the optimization" is actually more > valuable than "did we do the optimization at all". Additionally I > think showing it as an optimization of an existing node is actually > likely less confusing anyway. > > One other thing: my understanding is that this actually matches the > underlying code split too. For the index only scan case, we actually > have a separate node (it's not just an optimization of the standard > index scan). There are discussions about whether that's a good thing, > but it's what we have. In contrast, the bitmap scan actually has it as > a pure optimization of the existing bitmap heap scan node. > I personally see those as valid points. Does anybody else want to weigh in here, so that we can reach to some conclusion and move ahead with this CF entry? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes:
> On Sat, Mar 28, 2020 at 8:02 PM James Coleman <jtc331@gmail.com> wrote:
>> I'm curious if Tom's objection is mostly on the grounds that we should
>> be consistent in what's displayed, or that he thinks the information
>> is likely to be useless.
> Yeah, it would be good if he clarifies his position.
Some of both: it seems like these ought to be consistent, and the
lack of complaints so far about regular index-only scans suggests
that people don't need the info. But perhaps we ought to add
similar info in both places.
regards, tom lane
On Mon, Mar 30, 2020 at 9:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Kapila <amit.kapila16@gmail.com> writes: > > On Sat, Mar 28, 2020 at 8:02 PM James Coleman <jtc331@gmail.com> wrote: > >> I'm curious if Tom's objection is mostly on the grounds that we should > >> be consistent in what's displayed, or that he thinks the information > >> is likely to be useless. > > > Yeah, it would be good if he clarifies his position. > > Some of both: it seems like these ought to be consistent, and the > lack of complaints so far about regular index-only scans suggests > that people don't need the info. But perhaps we ought to add > similar info in both places. > Fair enough. I have marked this CF entry as RWF. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com