Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vugwl-001kwe-2T for pgsql-hackers@arkaria.postgresql.org; Tue, 24 Feb 2026 01:10:44 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vugwk-00GA3Q-2U for pgsql-hackers@arkaria.postgresql.org; Tue, 24 Feb 2026 01:10:42 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vugwk-00GA3F-15 for pgsql-hackers@lists.postgresql.org; Tue, 24 Feb 2026 01:10:42 +0000 Received: from mail-pf1-x442.google.com ([2607:f8b0:4864:20::442]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1vugwg-00000000sjk-2dNM for pgsql-hackers@lists.postgresql.org; Tue, 24 Feb 2026 01:10:41 +0000 Received: by mail-pf1-x442.google.com with SMTP id d2e1a72fcca58-824c9da9928so2431955b3a.3 for ; Mon, 23 Feb 2026 17:10:39 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1771895439; cv=none; d=google.com; s=arc-20240605; b=kS8tOQIEHDAa65FHtNbXYg3QSDSfnYeRogNgHNB2P6l5/xLGGagYUqJ6+WJenwHU0y 24NOWG1vAI4h6zh0YyKcadL06rqcJFC8VfkpFBnMgLPDyhhZUHIa511ZGeHhC0ShpqT9 Qhl/RRz29iT3DFsol6JsQzqmNYaZp1RFPNOHM0kBHuR8UHdZONEkzc0s0g+mUaQGdQA7 wyRzUQHbqKCIxAByyZ5YwNW7vE30Pl3W8YAmBQ0xCr5JLE1FWuNbpp1cRqBxWkiusmqD yuS95K6z7kKTC8fsWhiz8HeQP6VlIpxarCqLHxduArkcXzv653m68WKdkhkf/paUa/QE OSRQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=Z474jUKz6/0Q7X4Cu2xGDT+wlPuyek8M99yTzncDaKw=; fh=TljxRdClaeaXZJtzJeHgyHXATBAZdqHkhdgpjKULxoY=; b=BPDV+8bLQuzYuaTY/Pje3pUqYiJSTcSTUAcTXmYs55HEcDg/fxK+v3HFzA+UIcKj7+ ebewH2NSql9zKf0RnTebTTU+1I4Vt8M5rVs/K8S2+ZB5zy1VhArCFgIhnB3HahPNuVP2 wsRP3hT1ZVZvjYI1BLDAzlySNrmmnNHiUAF05j6O04ktpLNs8en/k+7brvNHIOcFVU/F /cYMGxUwYKlJiqei7odiNxClPgLVyJobM6gdB1rX3iNJH937pxU5SHjc0c/+ZrimsRRM 8ISYn922j3TS6CiCp4wPJ2q+Q5Klok6VPBes5+DxMivSLy2mJR08Ct/WjnDr5zeIxPHQ EqvA==; darn=lists.postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1771895439; x=1772500239; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Z474jUKz6/0Q7X4Cu2xGDT+wlPuyek8M99yTzncDaKw=; b=JeCG0Jt9088N2GhRjM3wDzspsMd7jQ7pASPSpLyte5FS3ACJb+xxJ4VnFO/FW2BH+N BKZdcyZX41N3UqtVz4LTZPyA7u/b05ymywmURAGefxY/BpILLxoTfvvC3fRpFQxWcgt0 vbf8gBqCAa7IDCK/aKu5w+0e2nG3WoKw8WYNxHyZ2CZfwCBX1D7D+Zlbp9QQspXrpklt DB0waJRIkeHD/KO8QCFa8+uR0zq62utrPFrZvj8po5ufLdMMzccz5/0GsrrWvXApNHEg 1UOiF/4fGpAyQcPnBKNs1eGz4ZPTl9QBK2sUOYORSoglAzEHr7L5Fvb0oXl8/UCBZkko 9cGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1771895439; x=1772500239; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Z474jUKz6/0Q7X4Cu2xGDT+wlPuyek8M99yTzncDaKw=; b=AMbiQZ5RD0nxySZz+2qtBfZsS0FXWUiZKtFTzCdStMIlNhEZpuCAY1IZFhepO25kle NyMI9ITZWi6IzSmzrp70B1qFXlplFYHyufvF/aMH4ZgPQ3ON5X/JoxiFxqxfltXr1fKl ceEN3lFffDKvGsDKFQuW5hJwdcmVszNx0SIOiff2CiKb8iel8w1TBwfPdRTwC1igI0i0 FJFo92zVYYZ8166Je+7tRlCdpxnCT7C29Ak1Nd+j3gCAjreaDyic46n1KFmPZclVEtnH JMgb4DeeSibcUD4ibsv3nc01AdVz8UfV/f7yJT/TJm5jyzonk4A9wp8m4EAJEgLNPo3C JPcQ== X-Forwarded-Encrypted: i=1; AJvYcCWmH3QIjqcfGMLqEABQmpqPqg2n5OetdL/c7Xv+ljkNwvtXi2UbQLhXIKH71DMBRIa6Rh34HXUlI2YDp10t@lists.postgresql.org X-Gm-Message-State: AOJu0YzxiwHjzevNBNLKDBNl0WONAb7uKShykesj8KIhYI7slzJrZKYH myrieDgsuZ/DZIBkjB1XWAAtqcADCcpFPLS1dOdjbB6ejwe0NNL/oSy3FQkzyADLFHOycMM05cY ldccz4FatuQIVa1pTG4Rchty6b/lyFJc= X-Gm-Gg: AZuq6aLHT0Z13Y2jhndU7utBamAa9y7Tt2PPvORQxqRM8IJPY39uTYilm+rqVf0qrpH zev4/lHleX/431FLdSaYLTRFNygiA/Q+iXzQS7a1J6ID12WjI5TOLXlVQp5wkHoYcFkK+jxP0Tu yaPdTm1/DOuPdqvyu0S82YWtQON1vITeCm1hOszDBjrK/adRYgGT8hSmAIiMMq48R8MYyqv21L9 cB1UQpXZrv70kgz2qizc8vYxqwYHu1Qbx55NGeJwmURnwAss4MIPjJcAj+4AyvXUXGTmNhHdd9d YfJt9qOgwA== X-Received: by 2002:a05:6a00:885:b0:81f:477d:58db with SMTP id d2e1a72fcca58-826daa0cddfmr11178270b3a.39.1771895438604; Mon, 23 Feb 2026 17:10:38 -0800 (PST) MIME-Version: 1.0 References: <1136161.1769654478@sss.pgh.pa.us> In-Reply-To: From: Alexandra Wang Date: Mon, 23 Feb 2026 17:10:02 -0800 X-Gm-Features: AaiRm51iCKPMZ9jo20vrisZPRQgFUQrnhUCm6KVjflpk3QXiAYKPpe8G2DMxqYw Message-ID: Subject: Re: pg_plan_advice To: Robert Haas Cc: Richard Guo , Lukas Fittl , Tom Lane , Jacob Champion , Dian Fay , Matheus Alcantara , Jakub Wartak , PostgreSQL Hackers Content-Type: multipart/alternative; boundary="000000000000327f72064b878e8e" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000327f72064b878e8e Content-Type: text/plain; charset="UTF-8" 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 --000000000000327f72064b878e8e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Robert,

I've been reviewing= the v17-0003 patch and learned a lot along the
way.=C2=A0 The design is= well thought out and the implementation looks
solid.=C2=A0 I only have = a few minor comments and questions.

While reading the patch, I notic= ed 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_joi= n():
> /*
> =C2=A0* Can we see a Result node here, to project a= bove a Gather? So far I've
> =C2=A0* found no example that behave= s that way; rather, the Gather or Gather
> =C2=A0* Merge is made to p= roject. Hence, don't test is_result_node_with_child()
> =C2=A0* a= t this point.
> =C2=A0*/

In pgpa_descend_any_unique():
>= else if (IsA(*plan, Agg))
> {
> /*
> * If this is a s= imple Agg node, then assume it's here to implement
> * semijoin= uniqueness. Otherwise, assume it's completing an eager
> * agg= regation or partitionwise aggregation operation that began at a
> *= higher level of the plan tree.
> *
> * (Note that when we&= #39;re using an Agg node for uniqueness, there's no
> * need fo= r any case other than AGGSPLIT_SIMPLE, because there's no
> * a= ggregated column being * computed. However, the fact that
> * AGGSP= LIT_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 cert= ain
> * of its analysis.)
> */
> descend =3D true;> sjunique =3D (((Agg *) *plan)->aggsplit =3D=3D AGGSPLIT_SIMPLE);=
> }

In pgpa_walk_recursively():
> =C2=A0* Exception: We= disregard any single_copy Gather nodes. These are created
> =C2=A0* = by debug_parallel_query, and having them affect the plan advice is
> = =C2=A0* counterproductive, as the result will be to advise the use of a rea= l
> =C2=A0* Gather node, rather than a single copy one.
> =C2= =A0*/
> if (IsA(plan, Gather) && !((Gather *) plan)->singl= e_copy)
> {
> active_query_features =3D
> lappend(list= _copy(active_query_features),
> pgpa_add_feature(walker, PGPAQF_G= ATHER, plan));
> beneath_any_gather =3D true;
> }

In pg= pa_build_scan():
> /*
> =C2=A0* If setrefs processing elided an= Append or MergeAppend node that had
> =C2=A0* only one surviving chi= ld, it might be a partitionwise operation,
> =C2=A0* but then this is= either a setop over subqueries, or a partitionwise
> =C2=A0* operati= on (which might be a scan or a join in reality, but here we
> =C2=A0*= don't care about the distinction and consider it simply a scan).
&g= t; =C2=A0*
> =C2=A0* A setop over subqueries, or a trivial SubQuerySc= an that was elided,
> =C2=A0* is an "ordinary" scan i.e. on= e for which we need to generate advice
> =C2=A0* because the planner = has not made any meaningful choice.
> =C2=A0*/
> if ((nodetype = =3D=3D T_Append || nodetype =3D=3D T_MergeAppend) &&
> uniqu= e_nonjoin_rtekind(relids,
> =C2=A0 walker->pstmt->rtable= ) =3D=3D RTE_RELATION)
> strategy =3D PGPA_SCAN_PARTITIONWISE;
&g= t; else
> strategy =3D PGPA_SCAN_ORDINARY;

All of the interpr= etations above look reasonable, but they made me
wonder how modules like= this detect planner-behavior drift over time
if we add new plan node ty= pes, add or change plan shapes, or even a
new debug GUC.

I realiz= e this isn't specific to this patch, but I'm curious whether
som= ething like monitoring a possible set of parent-child plan node
patterns= has ever been considered, to make structural changes easier
to notice w= hen those invariants stop holding. I don't think this is
actionable = for this patch, but I'm just recording the thought here
while readin= g.

---

I ran the pg_plan_advice module tests as well as test_= plan_advice, and
everything passed locally.

I also noticed that s= everal tests in pg_plan_advice are not listed in
the Makefile. Specifica= lly, they are semijoin.sql, syntax.sql,
prepared.sql, and local_collecto= r.sql. I ran them manually, and they
all passed.

---

In pg= pa_walk_recursively():
> /*
> =C2=A0* Check the future_query_fe= atures list to see whether this was previously
> =C2=A0* identified a= s a plan node that needs to be treated as a query feature.
> =C2=A0* = We must do this before handling elided nodes, because if there's an
= > =C2=A0* elided node associated with a future query feature, the RTIs a= ssociated
> =C2=A0* with the elided node should be the only ones attr= ibuted to the query
> =C2=A0* feature.
> =C2=A0*/
> forea= ch_ptr(pgpa_query_feature, qf, walker->future_query_features)
> {<= br>> if (qf->plan =3D=3D plan)
> {
> active_query_fea= tures =3D list_copy(active_query_features);
> active_query_features= =3D lappend(active_query_features, qf);
> walker->future_query_= features =3D
> list_delete_ptr(walker->future_query_features, p= lan);
> break;
> }
> }

Should this be deleting = "qf" instead of "plan"? Right now the list is
not em= ptied after the plan tree walk as intended.

---

In pgpa_decom= pose_join():
> if (found_any_outer_gather &&
> ...
= > if (found_any_inner_gather &&

"found_any_{outer,in= ner}_gather" should be dereferenced.

---

In pgpa_trim_sh= ared_advice():
> /* Don't leave stale pointers around. */
>= memset(&chunk_array[remaining_chunk_count], 0,
> =C2=A0 sizeof= (pgpa_shared_advice_chunk *)
> =C2=A0 * (total_chunk_count - remain= ing_chunk_count));

Should the element size be "sizeof(dsa_point= er)", consistent with the
preceding memmove()?

---

In= pg_plan_advice_advice_check_hook():
> if (error !=3D NULL)
> {=
> GUC_check_errdetail("Could not parse advice: %s", error= ); //
> return false;
> }
>
> MemoryContextSwitch= To(oldcontext);
> MemoryContextDelete(tmpcontext);
>
> re= turn 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:

&= gt; 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 bu= f;" is unused.

---

The "ExplainState *explain_state= " field and the "MemoryContext
trove_cxt" field in "= struct pgpa_planner_state" are both unused.

Best,
Alex

--
--000000000000327f72064b878e8e--