public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexandra Wang <[email protected]>
To: Robert Haas <[email protected]>
Cc: Richard Guo <[email protected]>
Cc: Lukas Fittl <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Jacob Champion <[email protected]>
Cc: Dian Fay <[email protected]>
Cc: Matheus Alcantara <[email protected]>
Cc: Jakub Wartak <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: pg_plan_advice
Date: Mon, 23 Feb 2026 17:10:02 -0800
Message-ID: <CAK98qZ1J42RoAsHnYWGPPmHziZmzmqE=Lp_O6WJ-9aKK2qjikA@mail.gmail.com> (raw)
In-Reply-To: <CA+Tgmob7ozJAs5SU6bD2RfAt4w_AmsMGz-aaVA6WeLXHkBypOg@mail.gmail.com>
References: <CA+TgmoZ-Jh1T6QyWoCODMVQdhTUPYkaZjWztzP1En4=ZHoKPzw@mail.gmail.com>
<CAKZiRmxtJAFG7e1+Vs9B8ngON=AOzJbuws+1ZeH4LsbJh5AzoQ@mail.gmail.com>
<CA+TgmoY9Ne_Sh10u6LSPc3wvOQPLp3kF9nBp3nqJEG2JGF2QiA@mail.gmail.com>
<CA+Tgmoa57S6mP=aTOXH2-gDAL4TMO1WbGgrHSg0s6J4zUH=04g@mail.gmail.com>
<[email protected]>
<CA+Tgmoaf__2B0BUL+vrg28P+3buX=Ti-kybqkHiLTtFrrCfzuA@mail.gmail.com>
<CA+TgmoYpcLNOuypOTdgCSLW7FuA=t6BtB3meTARHX2-Dj_81xQ@mail.gmail.com>
<[email protected]>
<CA+TgmoZjv9OyFu1Gkt78w0vWEti8S33w8trYHmErf-GMmGSi=w@mail.gmail.com>
<[email protected]>
<CA+TgmoaOSBQD9Ux4eG40w723ZN=c0J7p-+oX4+J8urUeyLMo5w@mail.gmail.com>
<CAOYmi+=g+MMoOpWkk2weXWKJcKH0eKey8gKHHdH0dF4Tiawrhw@mail.gmail.com>
<CA+TgmobwaT=PXPDDrgDup+jA8KHBbkxigtziD-zNzAKKkQYVgQ@mail.gmail.com>
<CAOYmi+mOmEW=amDRQMfw6-Fb3ZmDEQFaJzwk8Bc8W8DzaP85XQ@mail.gmail.com>
<CA+TgmoaX2AMW4cdFM3OngBJxmxpkdmzF33R7-CWhvRLfucbFMg@mail.gmail.com>
<CAOYmi+k4AyWCQHK=XVF99KVDuFkqxcADao61OWGLxu0nRYMONQ@mail.gmail.com>
<CA+TgmoZ0x3ym_oueXRWzbM_=6ucKoPZVGj3rRMLBDC_FnetXDw@mail.gmail.com>
<CAP53Pkycc=7N2bLzVT3x+qE1JamvRZWev5tFjdLJ1+-AV3Di+Q@mail.gmail.com>
<CA+TgmoaKhuD91RnazbRyGkmP7--JdNq8oNDC3UcgTZSWbMxC7w@mail.gmail.com>
<CAP53Pkw5-wMEeDJXFmqo_RTyL_spzCXb7HHKrbSnQqokVoZcNQ@mail.gmail.com>
<CA+Tgmob-69bzbdi3U_QtebqAf6u1y8js=5=oNK639csVe1VbhA@mail.gmail.com>
<CA+TgmoaZMOikxK=LqS+Jn+835h9S139JLGk-3LyETVXw5W5j=w@mail.gmail.com>
<[email protected]>
<CAP53PkwZ1ZTMARKg6iEfAw9qzBhkjBitj-9gr_Jvy7k2AwGgWA@mail.gmail.com>
<CAMbWs4--NuEUFE_xTo991TRXaZryE29jarJPDnVxoaQOYdt7tA@mail.gmail.com>
<CA+TgmobzR+XMGbRosVPbjHbSo4+cgJn=qZK6w05aF1sbj=C+9Q@mail.gmail.com>
<CA+TgmoawzvCoZAwFS85tE5+c8vBkqgcS8ZstQ_ohjXQ9wGT9sw@mail.gmail.com>
<CA+TgmoYS4ZCVAF2jTce=bMP0Oq_db_srocR4cZyO0OBp9oUoGg@mail.gmail.com>
<CAK98qZ2RzbgCHrSg4zLkvpzyBam_X6te-KF8w1+_vON9BAVMEw@mail.gmail.com>
<CA+TgmoaCdsuvNn6T6SfQ_0YD2Hh2+hgTXh9fTGHQhPg1zvy2rQ@mail.gmail.com>
<CAK98qZ1zWzRB0ABG7ULzTeWKRR5C7-FxLqM-6v8wQDiFM+DNAg@mail.gmail.com>
<CA+Tgmob7ozJAs5SU6bD2RfAt4w_AmsMGz-aaVA6WeLXHkBypOg@mail.gmail.com>
Hi Robert,
I've been reviewing the v17-0003 patch and learned a lot along the
way. The design is well thought out and the implementation looks
solid. I only have a few minor comments and questions.
While reading the patch, I noticed several places where we infer
planner intent from currently observed plan shapes (e.g., Gather
projection, Agg used for semijoin uniqueness, Append elision, etc.),
which I quote below:
In pgpa_decompose_join():
> /*
> * Can we see a Result node here, to project above a Gather? So far I've
> * found no example that behaves that way; rather, the Gather or Gather
> * Merge is made to project. Hence, don't test is_result_node_with_child()
> * at this point.
> */
In pgpa_descend_any_unique():
> else if (IsA(*plan, Agg))
> {
> /*
> * If this is a simple Agg node, then assume it's here to implement
> * semijoin uniqueness. Otherwise, assume it's completing an eager
> * aggregation or partitionwise aggregation operation that began at a
> * higher level of the plan tree.
> *
> * (Note that when we're using an Agg node for uniqueness, there's no
> * need for any case other than AGGSPLIT_SIMPLE, because there's no
> * aggregated column being * computed. However, the fact that
> * AGGSPLIT_SIMPLE is in use doesn't prove that this Agg is here for
> * the semijoin uniqueness. Maybe we should adjust an Agg node to
> * carry a "purpose" field so that code like this can be more certain
> * of its analysis.)
> */
> descend = true;
> sjunique = (((Agg *) *plan)->aggsplit == AGGSPLIT_SIMPLE);
> }
In pgpa_walk_recursively():
> * Exception: We disregard any single_copy Gather nodes. These are created
> * by debug_parallel_query, and having them affect the plan advice is
> * counterproductive, as the result will be to advise the use of a real
> * Gather node, rather than a single copy one.
> */
> if (IsA(plan, Gather) && !((Gather *) plan)->single_copy)
> {
> active_query_features =
> lappend(list_copy(active_query_features),
> pgpa_add_feature(walker, PGPAQF_GATHER, plan));
> beneath_any_gather = true;
> }
In pgpa_build_scan():
> /*
> * If setrefs processing elided an Append or MergeAppend node that had
> * only one surviving child, it might be a partitionwise operation,
> * but then this is either a setop over subqueries, or a partitionwise
> * operation (which might be a scan or a join in reality, but here we
> * don't care about the distinction and consider it simply a scan).
> *
> * A setop over subqueries, or a trivial SubQueryScan that was elided,
> * is an "ordinary" scan i.e. one for which we need to generate advice
> * because the planner has not made any meaningful choice.
> */
> if ((nodetype == T_Append || nodetype == T_MergeAppend) &&
> unique_nonjoin_rtekind(relids,
> walker->pstmt->rtable) == RTE_RELATION)
> strategy = PGPA_SCAN_PARTITIONWISE;
> else
> strategy = PGPA_SCAN_ORDINARY;
All of the interpretations above look reasonable, but they made me
wonder how modules like this detect planner-behavior drift over time
if we add new plan node types, add or change plan shapes, or even a
new debug GUC.
I realize this isn't specific to this patch, but I'm curious whether
something like monitoring a possible set of parent-child plan node
patterns has ever been considered, to make structural changes easier
to notice when those invariants stop holding. I don't think this is
actionable for this patch, but I'm just recording the thought here
while reading.
---
I ran the pg_plan_advice module tests as well as test_plan_advice, and
everything passed locally.
I also noticed that several tests in pg_plan_advice are not listed in
the Makefile. Specifically, they are semijoin.sql, syntax.sql,
prepared.sql, and local_collector.sql. I ran them manually, and they
all passed.
---
In pgpa_walk_recursively():
> /*
> * Check the future_query_features list to see whether this was previously
> * identified as a plan node that needs to be treated as a query feature.
> * We must do this before handling elided nodes, because if there's an
> * elided node associated with a future query feature, the RTIs associated
> * with the elided node should be the only ones attributed to the query
> * feature.
> */
> foreach_ptr(pgpa_query_feature, qf, walker->future_query_features)
> {
> if (qf->plan == plan)
> {
> active_query_features = list_copy(active_query_features);
> active_query_features = lappend(active_query_features, qf);
> walker->future_query_features =
> list_delete_ptr(walker->future_query_features, plan);
> break;
> }
> }
Should this be deleting "qf" instead of "plan"? Right now the list is
not emptied after the plan tree walk as intended.
---
In pgpa_decompose_join():
> if (found_any_outer_gather &&
> ...
> if (found_any_inner_gather &&
"found_any_{outer,inner}_gather" should be dereferenced.
---
In pgpa_trim_shared_advice():
> /* Don't leave stale pointers around. */
> memset(&chunk_array[remaining_chunk_count], 0,
> sizeof(pgpa_shared_advice_chunk *)
> * (total_chunk_count - remaining_chunk_count));
Should the element size be "sizeof(dsa_pointer)", consistent with the
preceding memmove()?
---
In pg_plan_advice_advice_check_hook():
> if (error != NULL)
> {
> GUC_check_errdetail("Could not parse advice: %s", error); //
> return false;
> }
>
> MemoryContextSwitchTo(oldcontext);
> MemoryContextDelete(tmpcontext);
>
> return true;
If an error occurs, do we leak the memory context? Should we also
switch and delete memory context before returning false?
---
In pg_get_collected_local_advice(), we skip rows when:
> if (!member_can_set_role(userid, ca->userid))
> continue;
Do we need to do the same check for pg_get_collected_shared_advice()?
---
In pgpa_planner_append_feedback(), "StringInfoData buf;" is unused.
---
The "ExplainState *explain_state" field and the "MemoryContext
trove_cxt" field in "struct pgpa_planner_state" are both unused.
Best,
Alex
--
Alexandra Wang
EDB: https://www.enterprisedb.com
view thread (143+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: pg_plan_advice
In-Reply-To: <CAK98qZ1J42RoAsHnYWGPPmHziZmzmqE=Lp_O6WJ-9aKK2qjikA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox