Re: On disable_cost

On Wed, Oct 2, 2024 at 8:11 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> I can't quite find the area you're talking about where the
> disabled_nodes don't propagate through subquery levels. Looking at
> cost_subqueryscan(), I see propagation of disabled_nodes. If the
> SubqueryScan node isn't present then the propagation just occurs
> normally as it does with other path types. e.g. master does:

Yeah, that case seems to work OK. But for example, consider this:

robert.haas=# explain with recursive foo as (select count(*) from
pgbench_accounts union all select aid from pgbench_accounts a, foo
where aid > foo.count) select * from pgbench_accounts, foo where aid =
foo.count;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------
Hash Join (cost=245288.11..348310.09 rows=3333331 width=105)
Disabled Nodes: 1
Hash Cond: (foo.count = pgbench_accounts.aid)
CTE foo
-> Recursive Union (cost=2890.00..239835.11 rows=3333331 width=8)
Disabled Nodes: 2
-> Aggregate (cost=2890.00..2890.01 rows=1 width=8)
Disabled Nodes: 1
-> Seq Scan on pgbench_accounts pgbench_accounts_1
(cost=0.00..2640.00 rows=100000 width=0)
Disabled Nodes: 1
-> Subquery Scan on "*SELECT* 2" (cost=369.02..20361.18
rows=333333 width=8)
Disabled Nodes: 1
-> Nested Loop (cost=369.02..16194.52 rows=333333 width=4)
Disabled Nodes: 1
-> WorkTable Scan on foo foo_1
(cost=0.00..0.20 rows=10 width=8)
-> Bitmap Heap Scan on pgbench_accounts a
(cost=369.02..1286.10 rows=33333 width=4)
Disabled Nodes: 1
Recheck Cond: (aid > foo_1.count)
-> Bitmap Index Scan on
pgbench_accounts_pkey (cost=0.00..360.69 rows=33333 width=0)
Index Cond: (aid > foo_1.count)
-> CTE Scan on foo (cost=0.00..66666.62 rows=3333331 width=8)
-> Hash (cost=2640.00..2640.00 rows=100000 width=97)
Disabled Nodes: 1
-> Seq Scan on pgbench_accounts (cost=0.00..2640.00
rows=100000 width=97)
Disabled Nodes: 1
(25 rows)

You might expect that the number of disabled nodes for the hash join
would include the number for the CTE attached to it, but it doesn't. I
suspect similar things will happen when a node has an InitPlan or
SubPlan node attached to it. (I have not tested whether your patch
gets these cases right.)

> I understand you're keen on keeping the output as it is in master. It
> would be good to hear if other people agree with you on this. I
> imagine you'd rather work on other things, but it's easier to discuss
> this now than after PG18 is out.

For sure. To be clear, it's not that I love the current output. It's
that I'm worried that it will be hard to get the thing that you and
Laurenz want to be fully reliable, and I think there's a chance that
not only might it contain bugs now, but it might turn out that people
changing logic in this area in the future introduce more bugs.
plan_is_disabled() has to get exactly the correct answer for the child
nodes every time, or the answer is wrong, and I'm not as confident as
you are that your logic is fully correct (which doesn't mean that I
can prove to you that it is incorrect; I don't even know that it is).
I agree that if we're going to change this, it's much better to do it
sooner rather than later, because then we've got time to debug it if
needed.

--
Robert Haas
EDB: http://www.enterprisedb.com