Re: On disable_cost

Jonathan S. Katz <jkatz@postgresql.org>

From: "Jonathan S. Katz" <jkatz@postgresql.org>
To: Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, David Rowley <dgrowleyml@gmail.com>, Peter Geoghegan <pg@bowt.ie>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, andrew@ankane.org
Date: 2024-08-23T21:33:12Z
Lists: pgsql-hackers
On 8/23/24 3:32 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I find both of your proposed solutions above to be pretty inelegant,
> 
> They are that.  If we were working in a green field I'd not propose
> such things ... but we aren't.  I believe there are now a fair number
> of out-of-core index AMs, so I'd rather not break all of them if we
> don't have to.

For distribution of index AMs in the wild, it's certainly > 1 now, and 
increasing. They're not the easiest extension types to build out, so 
it's not as widely distributed as some of the other APIs, but there are 
a bunch out there, as well as language-specific libs (e.g. pgrx for 
Rust) that offer wrappers around them.

>> and I think if this problem occurred with a core AM, I'd push for an
>> API break rather than accept the ugliness. "This path is not valid
>> because the AM cannot support it", "this path is crazy expensive", and
>> "the user told us not to do it this way" are three different things,
>> and signalling two or more of them in the same way muddies the water
>> in a way that I don't like.
> 
> I think it's not that bad, because we can limit the knowledge of this
> hack to the amcostestimate interface, which doesn't really deal in
> "the user told us not to do it this way" at all.  That argues against
> my first proposal though (having amcostestimate touch disabled_nodes
> directly).  I now think that a reasonable compromise is to say that
> setting indexTotalCost to +Inf signals that "the AM cannot support
> it".  That's not conflated too much with the other case, since even a
> crazy-expensive cost estimate surely ought to be finite.  We can have
> cost_index untangle that case into a separate failure return so that
> the within-the-core-optimizer APIs remain clean.
> 
> While that would require hnsw to make a small code change (return
> +Inf not DBL_MAX), that coding should work in back branches too,
> so they don't even need a version check.

+1 for this approach (I'll do a quick test in my pgvector workspace just 
to ensure it gets the same results in the older version).

Jonathan

Commits

  1. Doc: add detail about EXPLAIN's "Disabled" property

  2. Adjust EXPLAIN's output for disabled nodes

  3. Fix order of parameters in a cost_sort call

  4. Show number of disabled nodes in EXPLAIN ANALYZE output.

  5. Treat number of disabled nodes in a path as a separate cost metric.

  6. Remove grotty use of disable_cost for TID scan plans.