Re: pg_plan_advice: add NO_ scan and join method tags

At 2026-07-28 00:45:08, "Florin Irion" <irionr(at)gmail(dot)com> wrote:
>
>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.

Hi,
I have a minor comment on the v3 patch.

diff --git a/contrib/pg_plan_advice/pgpa_planner.c b/contrib/pg_plan_advice/pgpa_planner.c
index b3329b793aa..a29500e0e1d 100644
--- a/contrib/pg_plan_advice/pgpa_planner.c
+++ b/contrib/pg_plan_advice/pgpa_planner.c

+           /* Handle NO_ join method advice. */
+           {
+                 uint64            my_no_join_mask;
+
+                 my_no_join_mask = pgpa_no_join_mask_from_advice_tag(entry->tag);
+                 if (my_no_join_mask != 0)
+                 {
+                       bool        permit;
+                       bool        restrict_method;
+
+                       /*
+                        * NO_ join tags impose the same join-order constraint as
+                        * positive ones: the target must be the inner rel. Reuse
+                        * pgpa_join_method_permits_join to enforce it.
+                        */
+                       permit = pgpa_join_method_permits_join(pjs->outer_count,
+                                                                              pjs->inner_count,
+                                                                              pjs->rids,
+                                                                              entry,
+                                                                              &restrict_method);
+                       if (!permit)
+                             jo_deny_indexes = bms_add_member(jo_deny_indexes, i);
+                       else if (restrict_method)
+                       {
+                             no_jm_indexes = bms_add_member(no_jm_indexes, i);
+                             no_join_mask |= my_no_join_mask;
+                       }
+                       continue;
+                 }
+           }

Regarding the semantics of NO_ tags:
NO_HASH_JOIN((a b)) means a hash join cannot be used when the join product of a and b appears on the inner side.

Consider the following scenario:
SET pg_plan_advice.advice = 'JOIN_ORDER(t4 ((t2 t3) t1)) NO_HASH_JOIN((t1 t2))';

pgpa_join_method_permits_join() matches the set of inner relations against the target.
The result is ITM_TARGETS_ARE_SUBSET and restrict_method=false.
When inner={a,b,c}, a and b are indeed together on the inner side as part of a larger join product.
Per the intended semantics, the restriction from the NO_ tag should take effect: a hash join would be used with an inner side containing {a,b}.

But the current implementation only enforces the constraint for ITM_EQUAL and skips the ITM_TARGETS_ARE_SUBSET case.

I think this may be an issue.

Yanli Song