EXPLAIN IndexOnlyScan shows disabled when enable_indexonlyscan=on

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]: /messages/by-id/CAApHDvoUUKi0JNv8jtZPfc_JkLs7FqymC5-DDUFNKnm4MMmPuQ@mail.gmail.com
worth opening the discussion about that again.

David

[1]: /messages/by-id/CAApHDvoUUKi0JNv8jtZPfc_JkLs7FqymC5-DDUFNKnm4MMmPuQ@mail.gmail.com

On Monday, October 21, 2024, David Rowley <dgrowleyml@gmail.com> wrote:

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.

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.

David J.

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?

David

On Monday, October 21, 2024, David Rowley <dgrowleyml@gmail.com> wrote:

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?

Will do tomorrow.

David J.

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

On Mon, Oct 21, 2024 at 7:20 PM David Rowley <dgrowleyml@gmail.com> wrote:

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.

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

Do you want to submit that in patch form?

Went with a slightly different wording that seems to flow better with the
xrefs I added between the two options.

David J.

On Wed, 23 Oct 2024 at 13:51, David G. Johnston
<david.g.johnston@gmail.com> wrote:

Went with a slightly different wording that seems to flow better with the xrefs I added between the two options.

-        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 all index-scan
related plan

I'm concerned about the wording "all index-scan related". It's not
that clear if that would include Bitmap Index Scans or not. I think
it's better to explicitly mention index-only-scans to make it clear
which nodes are affected.

+        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?

-        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've attached that in patch form.

David

On Mon, Oct 28, 2024 at 3:54 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Wed, 23 Oct 2024 at 13:51, David G. Johnston
<david.g.johnston@gmail.com> wrote:

Went with a slightly different wording that seems to flow better with

the xrefs I added between the two options.

-        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 all index-scan
related plan

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 node types with the word index in them are not
affected.

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 bitmaps from them would be
precluded.

I could see pointing out, in enable_bitmapscan, that enable_bitmapscan is
effectively disabled (for index inputs) when enable_indexscan is set to
off. Then, in enable_indexscan, add a "see also" to enable_bitmapscan with
a brief reason as well.

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

+        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.

David J.

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 node types 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 bitmaps from them would be precluded.

I could see pointing out, in enable_bitmapscan, that enable_bitmapscan is effectively disabled (for index inputs) when enable_indexscan is set to off. Then, in enable_indexscan, add a "see also" to enable_bitmapscan with a brief reason as 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 are affected 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 Mon, Oct 28, 2024 at 6:03 PM David Rowley <dgrowleyml@gmail.com> wrote:

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 node types 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.

The documentation presently uses the term "index-scan related" and it is
unclear what exactly that is supposed to cover. My addition of the word
"all" doesn't materially change this other than for certain covering the
"index-only-scan related" nodes that gets clarified and is
cross-referenced. If you are uncertain whether adding "all" is meant to
cover Bitmap Index Scans then your uncertainty still exists in the current
wording. I just added "all" to be explicit about that fact, or at least
that is what I thought I did.

For me, the answer to "are bitmap index scans disabled" by setting
enable_indexscans to off is "yes" and does not require explanation. If the
real answer is "no" then please propose a change that can disabuse me of my
belief.

Is there a listing of all node types produced by PostgreSQL (with the

explain output naming) along with which ones are affected 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.

For education, mainly mine, not to add to the documentation; though our
lack of detail here for what are user-facing things is IMO unfortunate.

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.

Fair point. I'm good with your proposed change here.

David J.

On Mon, Oct 28, 2024 at 3:54 PM David Rowley <dgrowleyml@gmail.com> wrote:

I've attached that in patch form.

 -        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 plan into 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 individual plan nodes, "index scan" and
"index-only scan".

The hyphenation still reads a bit odd but ok.

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

David J.

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 plan into 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 individual plan 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]/messages/by-id/ccbe8ab940da76d388af7fc3fd169f1dedf751f6.camel@cybertec.at, 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. Though probably that something would fit better in an overview page rather than trying to get the settings to explain all this 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]: /messages/by-id/ccbe8ab940da76d388af7fc3fd169f1dedf751f6.camel@cybertec.at