Re: pg_plan_advice: add NO_ scan and join method tags

Hello,

On 14/07/2026 14:19, Jonathan Gonzalez V. wrote:
> I was trying to measure the test coverage of this patch and I found that
> the meson configuration wasn't updated, I think you're missing something
> like this:
>
> --- a/contrib/pg_plan_advice/meson.build
> +++ b/contrib/pg_plan_advice/meson.build
> @@ -57,6 +57,8 @@ tests += {
> 'gather',
> 'join_order',
> 'join_strategy',
> + 'no_join',
> + 'no_scan',
> 'partitionwise',
> 'prepared',
> 'scan',

Thanks for catching the meson.build gap — confirmed, no_join/no_scan
were added to the Makefile's REGRESS list but never mirrored into
meson.build. Fixed.

>
> After the tests run only two cases were not tested NO_MERGE_JOIN and
> NO_NESTED_LOOP, but these supposed to be tested, well I'll dig more on
> this one a second pass of the review.

On the coverage gap for NO_MERGE_JOIN/NO_NESTED_LOOP: I instrumented
both switch cases in pgpa_walker.c with log probes and reran no_join.sql
— both fire. Concretely: NO_NESTED_LOOP(f) (sql/no_join.sql:38) and the
stacked NO_HASH_JOIN(f) NO_NESTED_LOOP(f) case (line 52) both hit the
PGPA_TAG_NO_NESTED_LOOP case; NO_MERGE_JOIN(d) (line 45) hits
PGPA_TAG_NO_MERGE_JOIN. So ISTM the underlying advice-tag handling is
exercised.

>
> During the check, this came to my attention:
>
> + case PGPA_TAG_NO_INDEX_ONLY_SCAN:
> + return PGS_INDEXONLYSCAN | PGS_CONSIDER_INDEXONLY;
>
> Reading the comments I got this:
>
> When PGS_CONSIDER_INDEXONLY is
> unset, we don't even consider index-only scans, and any such scans that
> would have been generated become index scans instead. On the other hand,
> unsetting PGS_INDEXSCAN or PGS_INDEXONLYSCAN causes generated paths of the
> corresponding types to be marked as disabled.
>
> The message is a bit confusing but, I think that
> `PGS_INDEXONLY` should be the only option here? Otherwise, my
> understanding is that it will disable also PGS_INDEXSCAN, which is in
> the next line to be disable.

On PGS_CONSIDER_INDEXONLY: I think this traces back to the pathnodes.h
comment:

▎ When PGS_CONSIDER_INDEXONLY is unset, we don't even consider
index-only scans, and any such scans that would have been generated
become index scans instead. On the other hand, unsetting PGS_INDEXSCAN
or PGS_INDEXONLYSCAN causes generated paths of the corresponding types
to be marked as disabled.

That comment is actually describing the behavior we want.
NO_INDEX_ONLY_SCAN's mask clears PGS_INDEXONLYSCAN |
PGS_CONSIDER_INDEXONLY, leaving PGS_INDEXSCAN untouched. Per the
comment, clearing PGS_CONSIDER_INDEXONLY doesn't disable a path — it
changes what check_index_only() builds in the first place, converting
the would-be Index-Only Scan into a regular Index Scan path. Since we
never touch PGS_INDEXSCAN, that converted path stays enabled. Drop
PGS_CONSIDER_INDEXONLY from the mask (your suggestion) and
check_index_only() still returns true, so the planner builds an actual
(disabled) Index-Only Scan with no regular Index Scan alternative for
that index at all.

I confirmed this with the mask reduced to just PGS_INDEXONLYSCAN, the
no_scan test that expects NO_INDEX_ONLY_SCAN to fall back to a plain
Index Scan instead produced a Bitmap Heap Scan. So ISTM the current code
is correct.

attaching v3 with the meson.build change and rebased on current master.

Cheers,
Florin
---
www.enterprisedb.com