Thread: EXPLAIN IndexOnlyScan shows disabled when enable_indexonlyscan=on

I was surprised today when I saw that with
enable_indexscan=off
enable_indexonlyscan=on
EXPLAIN prints that the index only scan is disabled:

                  QUERY PLAN
-----------------------------------------------
 Index Only Scan using history_pkey on history
   Disabled: true

I wasn't sure if this was expected -- maybe index-only scan is
considered a type of index scan for this purpose. I dug around this
disable_cost thread [1] a bit to see if I could figure out what the
expected behavior is on my own, but I'm still not sure.

Anyway, maybe I'm misunderstanding something.

Here's my repro:

CREATE TABLE history(
    id INT PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY,
    data TEXT);
INSERT INTO history(data)
    select repeat('a', 100) from generate_series(1,10000)i;
VACUUM history;
set enable_seqscan = off;
set enable_indexscan = off;
set enable_bitmapscan = off;
set enable_indexonlyscan = on;
EXPLAIN (costs off) SELECT id from history;

- Melanie

[1]
https://www.postgresql.org/message-id/flat/CA%2BTgmoZEg1tyW31t3jxhvDwff29K%3D2C9r6722SuFb%3D3XVKWkow%40mail.gmail.com#402856db473920b9e0193b9f2cc2739b

On Tue, 22 Oct 2024 at 13:45, Melanie Plageman
<melanieplageman@gmail.com> wrote:
> I was surprised today when I saw that with
> enable_indexscan=off
> enable_indexonlyscan=on
> EXPLAIN prints that the index only scan is disabled:
>
>                   QUERY PLAN
> -----------------------------------------------
>  Index Only Scan using history_pkey on history
>    Disabled: true

There's nothing new about Index Only Scans being disabled by
enable_indexscan. Index Only Scan is chosen with your test case as all
possible Paths are disabled and IOS is the cheapest of all Paths.

The PG17 results for your test case are:

                                               QUERY PLAN
---------------------------------------------------------------------------------------------------------
 Index Only Scan using history_pkey on history
(cost=10000000000.29..10000000527.78 rows=19966 width=4)
(1 row)

(note the 1e10 disable_cost has been applied to the Index Only Scan costs)

Robert did propose to change this behaviour while he was working on
the disabled_nodes changes. I did push back on the proposed change
[1]. If you feel strongly that what we have is wrong, then maybe it's
worth opening the discussion about that again.

David

[1] https://www.postgresql.org/message-id/CAApHDvoUUKi0JNv8jtZPfc_JkLs7FqymC5-DDUFNKnm4MMmPuQ%40mail.gmail.com

On Tue, 22 Oct 2024 at 13:45, Melanie Plageman
<melanieplageman@gmail.com> wrote:
> I was surprised today when I saw that with
> enable_indexscan=off
> enable_indexonlyscan=on

Robert did propose to change this behaviour while he was working on
the disabled_nodes changes. I did push back on the proposed change
[1]. If you feel strongly that what we have is wrong, then maybe it's
worth opening the discussion about that again.

On Tue, 22 Oct 2024 at 14:46, David G. Johnston
<david.g.johnston@gmail.com> wrote:
> We should probably at least improve the documentation in 19.17.1; this interaction is apparently not self-evident.
>
> enable_indexscan
>
> Enable or disable the planner’s use of both index-scan and index-only-scans plan types.
>
> enabled_indexonlyscan
>
> Set to off to disable index-only-scan plan type while leaving index-scan plan types enabled.  This setting has no
effectif enable_indexscan is set to off. 

Yeah, I agree. The documentation could better reflect the current behaviour.

Do you want to submit that in patch form?

David

On Tue, 22 Oct 2024 at 14:46, David G. Johnston
<david.g.johnston@gmail.com> wrote:
> We should probably at least improve the documentation in 19.17.1; this interaction is apparently not self-evident.
>
> enable_indexscan
>
> Enable or disable the planner’s use of both index-scan and index-only-scans plan types.
>
> enabled_indexonlyscan
>
> Set to off to disable index-only-scan plan type while leaving index-scan plan types enabled.  This setting has no effect if enable_indexscan is set to off.

Yeah, I agree. The documentation could better reflect the current behaviour.

Do you want to submit that in patch form?

On Mon, Oct 21, 2024 at 9:32 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> There's nothing new about Index Only Scans being disabled by
> enable_indexscan. Index Only Scan is chosen with your test case as all
> possible Paths are disabled and IOS is the cheapest of all Paths.

Ah, I see! Sorry, I didn't think to compare and see what the cheapest
path would be if all paths were disabled and, of course, planner still
needs to pick one. Thanks!

> Robert did propose to change this behaviour while he was working on
> the disabled_nodes changes. I did push back on the proposed change
> [1]. If you feel strongly that what we have is wrong, then maybe it's
> worth opening the discussion about that again.

I suppose if you think of index-only scan as a kind of subclass of
index scan, then disabling index scan should disable index-only scan.
However, it seems like there should be a way to force an index-only
scan even if it is not the cheapest path. Perhaps I only think this as
a developer needing to test something. But if enable_indexscan
disables index-only scan then I don't see how I can force an
index-only scan when it is not cheapest.

- Melanie

On Wed, 23 Oct 2024 at 02:08, Melanie Plageman
<melanieplageman@gmail.com> wrote:
> However, it seems like there should be a way to force an index-only
> scan even if it is not the cheapest path. Perhaps I only think this as
> a developer needing to test something. But if enable_indexscan
> disables index-only scan then I don't see how I can force an
> index-only scan when it is not cheapest.

The way to do that is to turn off enable_seqscan and enable_bitmapscan
and ensure your query can support IOS.

The important part to remember here is that when creating the index
paths, the Index Only Scan is always assumed to be better than Index
Scan whenever it's possible to use IOS. There's no opportunity that if
an IOS is possible that you'll get an Index Scan instead.  See
check_index_only() and build_index_paths() around where
check_index_only() is used.

David

On Tue, Oct 22, 2024 at 3:21 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Wed, 23 Oct 2024 at 02:08, Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > However, it seems like there should be a way to force an index-only
> > scan even if it is not the cheapest path. Perhaps I only think this as
> > a developer needing to test something. But if enable_indexscan
> > disables index-only scan then I don't see how I can force an
> > index-only scan when it is not cheapest.
>
> The way to do that is to turn off enable_seqscan and enable_bitmapscan
> and ensure your query can support IOS.
>
> The important part to remember here is that when creating the index
> paths, the Index Only Scan is always assumed to be better than Index
> Scan whenever it's possible to use IOS. There's no opportunity that if
> an IOS is possible that you'll get an Index Scan instead.  See
> check_index_only() and build_index_paths() around where
> check_index_only() is used.

Ah, okay, I see. Thank you :)

- Melanie

We don't seem to be agreeing on much here... :-(

On Tue, 29 Oct 2024 at 13:30, David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Mon, Oct 28, 2024 at 3:54 PM David Rowley <dgrowleyml@gmail.com> wrote:
>> I'm concerned about the wording "all index-scan related".  It's not
>> that clear if that would include Bitmap Index Scans or not.
>
>
> That was partially the point of writing "all" there - absent other information, and seeing how index-only scans were
treated,I presumed it was indeed actually or effectively a switch for all.  If it is not it should be made clear which
nodetypes with the word index in them are not affected. 

I'm very much against mentioning which things are *not* affected by
settings. It doesn't seem like a very sustainable way to write
documentation.

>> I think
>> it's better to explicitly mention index-only-scans to make it clear
>> which nodes are affected.
>
> I hadn't considered Bitmap Index Scans but I would expect if you do not use index scans then the ability to produce
bitmapsfrom them would be precluded. 
>
> I could see pointing out, in enable_bitmapscan, that enable_bitmapscan is effectively disabled (for index inputs)
whenenable_indexscan is set to off.  Then, in enable_indexscan, add a "see also" to enable_bitmapscan with a brief
reasonas well. 

I don't follow this. enable_bitmapscan is completely independent from
enable_indexscan.

> Is there a listing of all node types produced by PostgreSQL (with the explain output naming) along with which ones
areaffected by which enable_* knobs (possibly multiple for something like Bitmap Index Scan)? 

No. We purposefully do our best not to document executor nodes. The
enable_* GUCs is one place where it's hard to avoid.

>>
>> +        types. The default is <literal>on</literal>. The
>> index-only-scan plan types
>> +        can be independently disabled by setting <xref
>> linkend="guc-enable-indexonlyscan"/>
>> +        to <literal>off</literal>.
>>
>> I wondered if it's better to reference the enable_indexonlyscan GUC
>> here rather than document what enable_indexonlyscan does from the
>> enable_indexscan docs. Maybe just a "Also see enable_indexonlyscans."
>> could be added?
>
>
> I prefer to briefly explain why we advise the reader to go "see also" here.
>
>>
>> -        The default is <literal>on</literal>.
>> +        The default is <literal>on</literal>. However, this setting
>> has no effect if
>> +        <xref linkend="guc-enable-indexscan"/> is set to
>> <literal>off</literal>.
>>
>> Could we just add "The <xref linkend="guc-enable-indexscan"/> setting
>> must also be enabled to have the query planner consider
>> index-only-scans"?
>
>
> I'd like to stick with a conjunction there but agree the "must be enabled" wording is preferrable, avoiding the
double-negative.
>
> "The default is on, but the <xref> setting must also be enabled."
>
> The 'to have the...' part seems to just be redundant.

I think it's confusing to include this as part of the mention of what
the default value is. The default value and enable_indexscans being
the master switch aren't at all related.

David

On Tue, 29 Oct 2024 at 14:41, David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Mon, Oct 28, 2024 at 3:54 PM David Rowley <dgrowleyml@gmail.com> wrote:
>  -        Enables or disables the query planner's use of index-scan plan
> -        types. The default is <literal>on</literal>.
> +        Enables or disables the query planner's use of index-scan and
> +        index-only-scan plan types.  The default is <literal>on</literal>.
> +        Also see <xref linkend="guc-enable-indexonlyscan"/>.
>
> I think the original wording "index-scan plan types" is what is confusing me.  The plural types is turning index-scan
planinto a category of plans rather than the single plan type "index scan". 
>
> Your proposed wording actually (accidentally?) fixes this because now the plural types actually refers to two
individualplan nodes, "index scan" and "index-only scan". 

I can't really vouch for the original wording as I didn't write it. I
agree the original use of "types" as a plural is strange and it's not
all that clear what that includes. Perhaps it was an attempt to mean
index and index-only scans

> The hyphenation still reads a bit odd but ok.

I'm not sure where the hyphenated form of "index-scan" comes from and
I admit that I blindly copied that form when I wrote
"index-only-scans". I'd much prefer we used <literal>Index
Scan</literal> and <literal>Index Only Scan</literal> so it could more
easily be matched up to what's shown in EXPLAIN. I don't think it's up
to this patch to change that, so I've just copied the existing form. I
was also warded off using the node name from EXPLAIN in [1], and on
checking the validity of the complaint, it seems valid.

> I am ok with this revision (and the patch as a whole) I suppose but I still feel like something is missing here.
Thoughprobably that something would fit better in an overview page rather than trying to get the settings to explain
allthis to the reader. 

Thanks. I'll go make it happen. It seems worthy of a backpatch because
it seems equally as applicable there, plus to maintain consistency.

For the part that seems missing... I'm not sure it's a great excuse,
but we've often been quite bad at updating the documentation when
making changes to the executor or EXPLAIN.  Tom fixed a bunch of stuff
in 5caa05749 which was outdated.  I think if we wanted to try and do a
better job of documenting plan choices and EXPLAIN output, we'd need
to consider if the said documentation is worth the additional
maintenance burden. It might be quite hard to decide that unless
someone went and wrote the documentation first so that we could
consider it on its own merit. Whoever does that would have to be
willing to have the whole work rejected if we decided it wasn't worth
the trouble. It seems like a bit of a thankless task and I'm not
motivated to do it. Your pain threshold might be higher than mine,
however.

David

[1] https://www.postgresql.org/message-id/ccbe8ab940da76d388af7fc3fd169f1dedf751f6.camel@cybertec.at