Re: On disable_cost

On Sat, Oct 5, 2024 at 1:37 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> On Sat, 5 Oct 2024 at 03:03, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > I tend to gravitate
> > toward displaying things exactly as they exist internally because I've
> > had so many bad experiences with having to try to reverse-engineer the
> > value stored internally from whatever is printed.
>
> Thanks for explaining your point of view. I've not shifted my opinion
> any, so I guess we just disagree. I feel a strong enough dislike for
> the current EXPLAIN output to feel it's worth working harder to have a
> better output.
>
> I won't push my point any further unless someone else appears
> supporting Laurenz and I. Thank you for working on getting rid of the
> disabled_cost. I think what we have is now much better than before.
> The EXPLAIN output is the only part I dislike about this work.
>
> I'd encourage anyone else on the sidelines who has an opinion on how
> to display the disabled-ness of a plan node in EXPLAIN to speak up
> now, even if it's just a +1 to something someone has already written.
> It would be nice to see what more people think.
>
> David
>

Hello,

Just like Laurenz, I was initially confused about what "Disabled Nodes"
means. Although I am not a Postgres hacker/committer, here is my $0.02c
perspective as a newcomer if it's useful (:-D):

- I appreciate Robert's concerns regarding the EXPLAIN output, which shows
information closely tied to how it is stored and planned by the planner
code, leaving less room for surprises.
- However, the EXPLAIN from `master` currently still throws me off. For
instance, consider the output below where the outer loop shows two
instances of `Disabled Nodes: 3` and the inner loop shows two instances of
`Disabled Nodes: 1`.

```
SET enable_hashjoin = off;
SET enable_mergejoin = off;
SET enable_indexscan = off;
SET enable_bitmapscan = off;
SET enable_nestloop = off;
SET enable_seqscan = off;

EXPLAIN (ANALYZE, COSTS ON)
SELECT *
FROM pg_class c
JOIN pg_attribute a ON c.oid = a.attrelid
WHERE c.relkind = 'r'
AND a.attnum > 0
LIMIT 10;

QUERY PLAN

---------------------------------------------------------------------------------------------------------------------------------------------------------------
Limit (cost=0.28..7.40 rows=10 width=511) (actual time=0.038..0.043
rows=10 loops=1)
Disabled Nodes: 3
-> Nested Loop (cost=0.28..290.15 rows=407 width=511) (actual
time=0.038..0.042 rows=10 loops=1)
Disabled Nodes: 3
-> Seq Scan on pg_class c (cost=0.00..19.19 rows=68 width=273)
(actual time=0.004..0.004 rows=1 loops=1)
Disabled Nodes: 1
Filter: (relkind = 'r'::"char")
-> Index Scan using pg_attribute_relid_attnum_index on
pg_attribute a (cost=0.28..3.92 rows=6 width=238) (actual
time=0.030..0.032 rows=10 loops=1)
Disabled Nodes: 1
Index Cond: ((attrelid = c.oid) AND (attnum > 0))
```

- In contrast, I think the PATCH's output is much clearer and makes reading
the plan more intuitive. It clearly indicates which nodes were disabled in
a nested output & plan, making it easier to count them if needed (using
grep, etc.).

Limit (cost=0.28..7.40 rows=10 width=511) (actual time=0.031..0.037
rows=10 loops=1)
-> Nested Loop (cost=0.28..290.15 rows=407 width=511) (actual
time=0.031..0.035 rows=10 loops=1)
Disabled: true
-> Seq Scan on pg_class c (cost=0.00..19.19 rows=68 width=273)
(actual time=0.011..0.011 rows=1 loops=1)
Disabled: true
Filter: (relkind = 'r'::"char")
-> Index Scan using pg_attribute_relid_attnum_index on
pg_attribute a (cost=0.28..3.92 rows=6 width=238) (actual
time=0.015..0.016 rows=10 loops=1)
Disabled: true
Index Cond: ((attrelid = c.oid) AND (attnum > 0))
Planning Time: 2.469 ms
Execution Time: 0.120 ms
(11 rows)

- Now, while the above output would make it easier for me as a
developer/user to understand the performance of my query and the decisions
planner took, I do appreciate Robert's concerns about tracing issues back
to the Postgres code if there is something wrong with the planner or
disabled logic itself. That said, not being able to replicate this behavior
with this PATCH is perhaps a good sign.

All that said, I still prefer the boolean attribute and placement under the
node. However, I think `Disabled: true` can still be confusing. `Disabled
Node: true/false` is clearer and leaves less room for conflict with other
features that might be disabled by the planner in the future.

Thanks
Shayon