Re: pg_plan_advice: add NO_ scan and join method tags

Re: pg_plan_advice: add NO_ scan and join method tags

Hello,

Adding v2 as it v1 doesn't apply anymore.


Cheers,
Florin
www.enterprisedb.com

Re: pg_plan_advice: add NO_ scan and join method tags

Hello!

> Hello,
>
> The attached patch adds negative scan- and join-method advice to
> pg_plan_advice. These tags are the complement of the existing
> positive ones:

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',

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.

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.

Regards,
--
Jonathan Gonzalez V.
EDB
https://www.enterprisedb.com

Re: pg_plan_advice: add NO_ scan and join method tags

Hi,

On 28/07/2026 08:43, song yanli wrote:
>
> 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 tested this using  JOIN_ORDER(t4 (t3 (t1 t2)))  with and without
NO_HASH_JOIN((t1 t2)):

- Where the inner side is exactly {t1,t2} (ITM_EQUAL), the tag fires:
   Hash Join → Merge Join there.
- Where the inner side is {t1,t2,t3} (ITM_TARGETS_ARE_SUBSET), the Hash Join
   stays, and EXPLAIN (PLAN_ADVICE) reports the tag as /* matched */.

Per the docs, HASH_JOIN((a b))/NO_HASH_JOIN((a b)) both refer to the join
product of exactly a and b — not any join whose inner side happens to 
contain
them plus other relations. pgpa_join_method_permits_join() is shared 
betwen the
positive and negative tags precisely to keep "NO_ is the logical 
complement of
the positive tag" true; enforcing the NO_ form more broadly tahn the 
positive
form's own matching scope would break that symmetry. The code comment 
says it
directly: for the TARGETS_ARE_SUBSET case, "HASH_JOIN((x y)) doesn't 
restrict
how x and y can be joined" — the join event being controlled is specifically
the one where {a,b} becomes someone else's inner side, not any join that 
merely
contains them.
On your exact example, JOIN_ORDER(t4 ((t2 t3) t1)) NO_HASH_JOIN((t1 t2)):
this isn't a silent bypass. That specific order requires joining t1 to 
the already
combined (t2 t3), which splits the {t1,t2} target across sides (t2 
merges with
outsider t3 before t1 and t2 join each other), so NO_HASH_JOIN's own 
join-order
logic denies that pairing, right where JOIN_ORDER demands it. Both tags come
back marked conflicting in the advice output, and the permit wins, so 
the plan
still follows your requested order.

>
> I think this may be an issue.
>

If you think this should be changed I think it's a design change that 
should be
discussed on a separate thread, what do you think?

Cheers,
Florin
www.enterprisedb.com

Re: pg_plan_advice: add NO_ scan and join method tags

Re: pg_plan_advice: add NO_ scan and join method tags

Hi Florin,
I tested the patch on current master, and it worked well in my testing.
Before applying the patch, advice such as NO_SEQ_SCAN(t1) was rejected
with a parser error because the NO_ scan and join tags were not
supported.
After applying the patch, the new NO_ tags were accepted successfully.
I verified that:
NO_SEQ_SCAN prevented the planner from using a Sequential Scan when
another eligible scan method was available.
NO_HASH_JOIN caused the planner to choose an alternative join method.
Multiple NO_ advice tags worked as expected.
Conflicting positive and negative advice for the same method was
detected correctly.
Positive and negative advice for different methods worked together as expected.
I also ran the regression tests, and everything completed successfully.

Thanks for working on this enhancement.

Regards

solai

Re: pg_plan_advice: add NO_ scan and join method tags

pgsql: Drop the per-entry copy of the flag mode in CompoundAffixFlags.

Drop the per-entry copy of the flag mode in CompoundAffixFlags.

Each CompoundAffixFlag carried its own copy of the dictionary's flag
mode, which decides whether the union holds a string or an integer.
The comment on the field explained why: cmpcmdflag() needs the mode,
and at the time there was no bsearch() variant that would allow
passing that context through.  But that problem is long gone, so let's
remove the redundant storage.  No flexibility is lost because things
would not work anyway if different flags within a dictionary have
different representations; and the preceding commit has seen to it
that they can't.  At present, this saves no storage on 64-bit machines
because of alignment issues, but it still seems like good
simplification.

No back-patch: there's little value in this except cleanliness, and
although probably no outside code is looking at this data structure,
this'd be an API/ABI break if any does.

Author: Ewan Young 
Reviewed-by: Tom Lane 
Discussion: https://postgr.es/m/CAON2xHN3QmsaySM6DGWa1gttcbJoFh0wjAE-_ZpSPo=LKN1hYw@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/2fb8da5a245661287833b05a1b2e275ddf83bbd7

Modified Files
--------------
src/backend/tsearch/spell.c       | 22 +++++++++-------------
src/include/tsearch/dicts/spell.h |  6 ++----
2 files changed, 11 insertions(+), 17 deletions(-)