Re: EXPLAIN IndexOnlyScan shows disabled when enable_indexonlyscan=on

On Tue, 29 Oct 2024 at 14:41, David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
> On Mon, Oct 28, 2024 at 3:54 PM David Rowley <dgrowleyml(at)gmail(dot)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], 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/ccbe8ab940da76d388af7fc3fd169f1dedf751f6.camel@cybertec.at