Subject: Re: EXPLAIN IndexOnlyScan shows disabled when enable_indexonlyscan=on

From: David Rowley <[email-masked]>
Date: Tue, 22 Oct 2024 14:32:45 +1300

On Tue, 22 Oct 2024 at 13:45, Melanie Plageman
<[email-masked]> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 21 Oct 2024 18:46:23 -0700

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

---

From: David Rowley <[email-masked]>
Date: Tue, 22 Oct 2024 15:20:22 +1300

On Tue, 22 Oct 2024 at 14:46, David G. Johnston
<[email-masked]> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 21 Oct 2024 19:40:11 -0700

> Do you want to submit that in patch form?

Will do tomorrow.

David J.

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 22 Oct 2024 17:50:56 -0700

> 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.
From 8b9be4096352088ea22f114e68041fd09ee6e28c Mon Sep 17 00:00:00 2001
From: "David G. Johnston" <[email-masked]>
Date: Tue, 22 Oct 2024 17:25:27 -0700
Subject: [PATCH] doc: Cross reference enable_indexscan and
 enable_indexonlyscan

Document that enable_indexscan controls the inclusion of all (normal
and index-only) index scan plan node types.  Note that index-only scans
can be independently disabled.

Note the implication of this in enable_indexonlyscan, that a setting of
"on" is ignored when all index scans types are disabled.

Left unwritten is the absence of a symmetric option to exclude normal
index scans while allowing index-only scans.
---
 doc/src/sgml/config.sgml | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 934ef5e469..04cbe7ac34 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5415,8 +5415,10 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
       </term>
       <listitem>
        <para>
-        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
+        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>.
        </para>
       </listitem>
      </varlistentry>
@@ -5429,9 +5431,10 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
       </term>
       <listitem>
        <para>
-        Enables or disables the query planner's use of index-only-scan plan
+        Set to <literal>off</literal> to disable the query planner's use of index-only-scan plan
         types (see <xref linkend="indexes-index-only-scans"/>).
-        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>.
        </para>
       </listitem>
      </varlistentry>
-- 
2.34.1

---

From: David Rowley <[email-masked]>
Date: Tue, 29 Oct 2024 11:54:01 +1300

On Wed, 23 Oct 2024 at 13:51, David G. Johnston
<[email-masked]> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 28 Oct 2024 17:29:28 -0700

On Mon, Oct 28, 2024 at 3:54 PM David Rowley <[email-masked]> wrote:

> > Went with a slightly different wording that seems to flow better with
> the xrefs I added between the two options.
>
> 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
> 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)?

> enable_indexscan docs. Maybe just a "Also see enable_indexonlyscans."

I prefer to briefly explain why we advise the reader to go "see also" here.

> must also be enabled to have the query planner consider

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.

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 28 Oct 2024 18:40:50 -0700

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

---

From: David Rowley <[email-masked]>
Date: Tue, 29 Oct 2024 16:06:25 +1300

On Tue, 29 Oct 2024 at 14:41, David G. Johnston
<[email-masked]> wrote:
> 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], 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] https://www.postgresql.org/message-id/[email-masked]