Re: On disable_cost - Mailing list pgsql-hackers

On 28/06/2024 18:46, Robert Haas wrote:
> On Wed, Jun 12, 2024 at 11:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> Well, that didn't generate much discussion, but here I am trying
>> again. Here I've got patches 0001 and 0002 from my previous posting;
>> I've dropped 0003 and 0004 from the previous set for now so as not to
>> distract from the main event, but they may still be a good idea.
>> Instead I've got an 0003 and an 0004 that implement the "count of
>> disabled nodes" approach that we have discussed previously. This seems
>> to work fine, unlike the approaches I tried earlier. I think this is
>> the right direction to go, but I'd like to know what concerns people
>> might have.
> 
> Here is a rebased patch set, where I also fixed pgindent damage and a
> couple of small oversights in 0004.
> 
> I am hoping to get these committed some time in July. So if somebody
> thinks that's too soon or thinks it shouldn't happen at all, please
> don't wait too long to let me know about that.

v3-0001-Remove-grotty-use-of-disable_cost-for-TID-scan-pl.patch:

+1, this seems ready for commit

v3-0002-Rationalize-behavior-of-enable_indexscan-and-enab.patch:

I fear this will break people's applications, if they are currently 
forcing a sequential scan with "set enable_indexscan=off". Now they will 
need to do "set enable_indexscan=off; set enable_indexonlyscan=off" for 
the same effect. Maybe it's acceptable, disabling sequential scans to 
force an index scan is much more common than the other way round.

v3-0003-Treat-number-of-disabled-nodes-in-a-path-as-a-sep.patch:

> @@ -1318,6 +1342,12 @@ cost_tidscan(Path *path, PlannerInfo *root,
>      startup_cost += path->pathtarget->cost.startup;
>      run_cost += path->pathtarget->cost.per_tuple * path->rows;
>  
> +    /*
> +     * There are assertions above verifying that we only reach this function
> +     * either when enable_tidscan=true or when the TID scan is the only legal
> +     * path, so it's safe to set disabled_nodes to zero here.
> +     */
> +    path->disabled_nodes = 0;
>      path->startup_cost = startup_cost;
>      path->total_cost = startup_cost + run_cost;
>  }

So if you have enable_tidscan=off, and have a query with "WHERE CURRENT 
OF foo" that is planned with a TID scan, we set disable_nodes = 0? That 
sounds wrong, shouldn't disable_nodes be 1 in that case? It probably 
cannot affect the rest of the plan, given that "WHERE CURRENT OF" is 
only valid in an UPDATE or DELETE, but still. At least it deserves a 
better explanation in the comment.

> diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
> index 6b64c4a362..20236e8c4d 100644
> --- a/src/backend/optimizer/plan/createplan.c
> +++ b/src/backend/optimizer/plan/createplan.c
> @@ -25,6 +25,7 @@
>  #include "nodes/extensible.h"
>  #include "nodes/makefuncs.h"
>  #include "nodes/nodeFuncs.h"
> +#include "nodes/print.h"
>  #include "optimizer/clauses.h"
>  #include "optimizer/cost.h"
>  #include "optimizer/optimizer.h"

left over from debugging?

> @@ -68,6 +68,15 @@ static bool pathlist_is_reparameterizable_by_child(List *pathlist,
>  int
>  compare_path_costs(Path *path1, Path *path2, CostSelector criterion)
>  {
> +    /* Number of disabled nodes, if different, trumps all else. */
> +    if (unlikely(path1->disabled_nodes != path2->disabled_nodes))
> +    {
> +        if (path1->disabled_nodes < path2->disabled_nodes)
> +            return -1;
> +        else
> +            return +1;
> +    }
> +
>      if (criterion == STARTUP_COST)
>      {
>          if (path1->startup_cost < path2->startup_cost)

Is "unlikely()" really appropriate here (and elsewhere in the patch)? If 
you run with enable_seqscan=off but have no indexes, you could take that 
path pretty often.

If this function needs optimizing, I'd suggest splitting it into two 
functions, one for comparing the startup cost and another for the total 
cost. Almost all callers pass a constant for that argument, so they 
might as well call the correct function directly and avoid the branch 
for that.

> @@ -658,6 +704,20 @@ add_path_precheck(RelOptInfo *parent_rel,
>          Path       *old_path = (Path *) lfirst(p1);
>          PathKeysComparison keyscmp;
>  
> +        /*
> +         * Since the pathlist is sorted by disabled_nodes and then by
> +         * total_cost, we can stop looking once we reach a path with more
> +         * disabled nodes, or the same number of disabled nodes plus a
> +         * total_cost larger than the new path's.
> +         */
> +        if (unlikely(old_path->disabled_nodes != disabled_nodes))
> +        {
> +            if (disabled_nodes < old_path->disabled_nodes)
> +                break;
> +        }
> +        else if (total_cost <= old_path->total_cost * STD_FUZZ_FACTOR)
> +            break;
> +
>          /*
>           * We are looking for an old_path with the same parameterization (and
>           * by assumption the same rowcount) that dominates the new path on
> @@ -666,39 +726,27 @@ add_path_precheck(RelOptInfo *parent_rel,
>           *
>           * Cost comparisons here should match compare_path_costs_fuzzily.
>           */
> -        if (total_cost > old_path->total_cost * STD_FUZZ_FACTOR)
> +        /* new path can win on startup cost only if consider_startup */
> +        if (startup_cost > old_path->startup_cost * STD_FUZZ_FACTOR ||
> +            !consider_startup)
>          {

The "Cost comparisons here should match compare_path_costs_fuzzily" 
comment also applies to the check on total_cost that you moved up. Maybe 
move up the comment to the beginning of the loop.

v3-0004-Show-number-of-disabled-nodes-in-EXPLAIN-ANALYZE-.patch:

It's surprising that the "Disable Nodes" is printed even with the COSTS 
OFF option. It's handy for our regression tests, it's good to print them 
there, but it feels wrong.

Could we cram it into the "cost=... rows=..." part? And perhaps a marker 
that a node was disabled would be more user friendly than showing the 
cumulative count? Something like:

postgres=# set enable_material=off;
SET
postgres=# set enable_seqscan=off;
SET
postgres=# set enable_bitmapscan=off;
SET
postgres=# explain select * from foo, bar;
                                      QUERY PLAN 

------------------------------------------------------------------------------------
  Nested Loop  (cost=0.15..155632.40 rows=6502500 width=8)
    ->  Index Only Scan using foo_i_idx on foo  (cost=0.15..82.41 
rows=2550 width=4)
    ->  Seq Scan on bar  (cost=0.00..35.50 (disabled) rows=2550 width=4)
(5 rows)

-- 
Heikki Linnakangas
Neon (https://neon.tech)