public inbox for [email protected]
help / color / mirror / Atom feedFrom: Robert Haas <[email protected]>
To: Tom Lane <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: Lukas Fittl <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: Melanie Plageman <[email protected]>
Cc: heikki.linnakangas <[email protected]>
Subject: Re: pg_plan_advice
Date: Tue, 7 Apr 2026 17:21:19 -0400
Message-ID: <CA+TgmobOOmmXSJz3e+cjTY-bA1+W0dqVDqzxUBEvGtW62whYGg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CA+TgmoZ-Jh1T6QyWoCODMVQdhTUPYkaZjWztzP1En4=ZHoKPzw@mail.gmail.com>
<CA+TgmoYjcBA6dw3nwiyfDzPXTCrxTZPXDMrc2TrDJcL1cPK6iA@mail.gmail.com>
<CA+TgmoYru-vxoTKfwjQby30r2OkTXfb18Km_=VLs6qk8Akr0-g@mail.gmail.com>
<CA+Tgmoau7yJtvbeH-0kPt1Q=Gt_ezRdgM35Q1=LT665U_86Etg@mail.gmail.com>
<[email protected]>
<CA+TgmobOLrMn5jEinWNPL5SrDH1DPpo3a4j+S6-4yhsZwWgzLg@mail.gmail.com>
<CA+TgmoZUN8FT1Ah=m6Uis5bHa4FUa+_hMDWtcABG17toEfpiUg@mail.gmail.com>
<CA+TgmoYh2-kM+tscOz=jVYq9Tf4SRPVqzPojs3KLZcW6E9m1BQ@mail.gmail.com>
<CA+TgmoaK=4w7-qknUo3QhUJ53pXZq=c=KgZmRyD+k7ytqfmgSg@mail.gmail.com>
<CAP53Pkz3DSFaaowYvbO5LULf3NhydD_UhHkighfWf6_pwxiqUw@mail.gmail.com>
<CA+TgmoZ45n5jaNKKgbbj4-kYV8WsPvUn=Z8HnoZ7tUb_p9WKXg@mail.gmail.com>
<CA+TgmoYuWmN-00Ec5pY7zAcpSFQUQLbgAdVWGR9kOR-HM-fHrA@mail.gmail.com>
<CAP53Pkzn_wZ-R-cPdD9XSQ9+myPUUsPMMqVBPNG3XWXhgfm1-Q@mail.gmail.com>
<CA+Tgmobxbju8PrY_NULtPr7b7UShp4+Jqibm2Bou8TVS69gObQ@mail.gmail.com>
<[email protected]>
<CA+TgmoadkuOMJjvYe2h6aznHFeePprGEQ8CgUpRK=47sB6DMAg@mail.gmail.com>
<[email protected]>
<CA+TgmoY+g1u-fN=3igXG-8u0Ho3V4u-ooWXCj-FQ9DA=uGek9g@mail.gmail.com>
<[email protected]>
On Fri, Apr 3, 2026 at 11:14 PM Tom Lane <[email protected]> wrote:
> I believe that we probably will need to do something in this
> area before v19 release. If we're willing to commit to it being
> "run the tests serially", then sure we can wait awhile before
> actually doing that. Maybe we'll even think of a better idea
> ... but what we can do about this post-freeze seems pretty
> constrained to me.
Here's a new patch set. All of these patches are new, but I'm
continuing to increment the same version number sequence.
0001 and 0002 implement the "retry a few times" idea for avoiding
test_plan_advice failures. I argue that (a) these are reasonable
post-commit stabilization that should not be blocked by feature freeze
and (b) most people here will be happier with a solution like this
that will normally cost very little than they will be with switching
test_plan_advice to executing serially. The RMT can decide whether it
agrees. The other question here is whether it's really a good idea to
apply this now considering that we've seen only one failure so far. I
think it's probably a good idea to do something like this before
release, so that we hopefully reduce the false positive rate from the
test to something much closer to zero, but I think we've still had
only the one failure, and I'm really interested in knowing how close
the failure rate is to zero already. The RMT may have an opinion on
how long to wait before doing something like this, too.
0003 fixes the problem with tablesample scans that Alexander Lakhin
reported. The bug occurs when a tablesample handler does not set
repeatable_across_scans and the resulting Sample Scan appears below a
join. The test case provided by Alexander shows the Sample Scan on the
inner side of the join, but it's also possible to construct a case
where it occurs on the outer side of the join. This commit adds tests
for both cases.
0004 fixes an oversight in commit
6455e55b0da47255f332a96f005ba0dd1c7176c2, which failed to add a new
pg_regress test to the pg_plan_advice Makefile.
0005 fixes the other issue that Alexander Lakhin recently reported,
which manifested as ERROR: no rtoffset for plan unnamed_subquery when
trying to generate advice. That turns out to occur when a subquery is
proven empty and that subquery contains a semijoin that could have
been implemented by making one side unique. I chose a different fix
than what I mentioned in my response to Alexander's email. There was
already code that handles the case where a SubPlanRTInfo exists and is
marked dummy, and this fix extends that handling to the case where no
SubPlanRTInfo exists at all, which seems better than treating those
two cases in separate parts of the code.
I don't think that anyone will argue that 0003-0005 are things we
can't or shouldn't fix after feature freeze, and I plan to apply those
fixes shortly after feature freeze, unless there are objections or
better ideas. I could rush them in before that, too, but I don't think
what the tree needs are more people trying to commit all at once right
now.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] v26-0001-pg_plan_advice-Export-feedback-related-definitio.patch (15.6K, 2-v26-0001-pg_plan_advice-Export-feedback-related-definitio.patch)
download | inline diff:
From cd2f70b2790c94d630a6e42526510f3c8e5c0b31 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Tue, 7 Apr 2026 12:39:21 -0400
Subject: [PATCH v26 1/5] pg_plan_advice: Export feedback-related definitions.
In preparation for making test_plan_advice slightly smarter, this
commit makes a couple of things related to pg_plan_advice visible
to other extensions. The PGPA_TE_* constants are renamed to
PGPA_FB_* and moved to pg_plan_advice.h. Also, the function
pgpa_planner_feedback_warning is made non-static and marked as
PGDLLEXPORT.
---
contrib/pg_plan_advice/pg_plan_advice.h | 29 +++++++++++
contrib/pg_plan_advice/pgpa_planner.c | 69 ++++++++++++-------------
contrib/pg_plan_advice/pgpa_planner.h | 3 ++
contrib/pg_plan_advice/pgpa_trove.c | 17 +++---
contrib/pg_plan_advice/pgpa_trove.h | 30 -----------
5 files changed, 75 insertions(+), 73 deletions(-)
diff --git a/contrib/pg_plan_advice/pg_plan_advice.h b/contrib/pg_plan_advice/pg_plan_advice.h
index 749331b6b8a..d7847715350 100644
--- a/contrib/pg_plan_advice/pg_plan_advice.h
+++ b/contrib/pg_plan_advice/pg_plan_advice.h
@@ -15,6 +15,35 @@
#include "commands/explain_state.h"
#include "nodes/pathnodes.h"
+/*
+ * Flags used in plan advice feedback.
+ *
+ * PGPA_FB_MATCH_PARTIAL means that we found some part of the query that at
+ * least partially matched the target; e.g. given JOIN_ORDER(a b), this would
+ * be set if we ever saw any joinrel including either "a" or "b".
+ *
+ * PGPA_FB_MATCH_FULL means that we found an exact match for the target; e.g.
+ * given JOIN_ORDER(a b), this would be set if we saw a joinrel containing
+ * exactly "a" and "b" and nothing else.
+ *
+ * PGPA_FB_INAPPLICABLE means that the advice doesn't properly apply to the
+ * target; e.g. INDEX_SCAN(foo bar_idx) would be so marked if bar_idx does not
+ * exist on foo. The fact that this bit has been set does not mean that the
+ * advice had no effect.
+ *
+ * PGPA_FB_CONFLICTING means that a conflict was detected between what this
+ * advice wants and what some other plan advice wants; e.g. JOIN_ORDER(a b)
+ * would conflict with HASH_JOIN(a), because the former requires "a" to be the
+ * outer table while the latter requires it to be the inner table.
+ *
+ * PGPA_FB_FAILED means that the resulting plan did not conform to the advice.
+ */
+#define PGPA_FB_MATCH_PARTIAL 0x0001
+#define PGPA_FB_MATCH_FULL 0x0002
+#define PGPA_FB_INAPPLICABLE 0x0004
+#define PGPA_FB_CONFLICTING 0x0008
+#define PGPA_FB_FAILED 0x0010
+
/* Hook for other plugins to supply advice strings */
typedef char *(*pg_plan_advice_advisor_hook) (PlannerGlobal *glob,
Query *parse,
diff --git a/contrib/pg_plan_advice/pgpa_planner.c b/contrib/pg_plan_advice/pgpa_planner.c
index b25f62b2e87..86b37e79b57 100644
--- a/contrib/pg_plan_advice/pgpa_planner.c
+++ b/contrib/pg_plan_advice/pgpa_planner.c
@@ -159,7 +159,6 @@ static List *pgpa_planner_append_feedback(List *list, pgpa_trove *trove,
pgpa_trove_lookup_type type,
pgpa_identifier *rt_identifiers,
pgpa_plan_walker_context *walker);
-static void pgpa_planner_feedback_warning(List *feedback);
static pgpa_planner_info *pgpa_planner_get_proot(pgpa_planner_state *pps,
PlannerInfo *root);
@@ -876,19 +875,19 @@ pgpa_planner_apply_joinrel_advice(uint64 *pgs_mask_p, char *plan_name,
* the set of targets exactly matched this relation, fully matched. If
* there was a conflict, mark them all as conflicting.
*/
- flags = PGPA_TE_MATCH_PARTIAL;
+ flags = PGPA_FB_MATCH_PARTIAL;
if (gather_conflict)
- flags |= PGPA_TE_CONFLICTING;
+ flags |= PGPA_FB_CONFLICTING;
pgpa_trove_set_flags(pjs->rel_entries, gather_partial_match, flags);
- flags |= PGPA_TE_MATCH_FULL;
+ flags |= PGPA_FB_MATCH_FULL;
pgpa_trove_set_flags(pjs->rel_entries, gather_full_match, flags);
/* Likewise for partitionwise advice. */
- flags = PGPA_TE_MATCH_PARTIAL;
+ flags = PGPA_FB_MATCH_PARTIAL;
if (partitionwise_conflict)
- flags |= PGPA_TE_CONFLICTING;
+ flags |= PGPA_FB_CONFLICTING;
pgpa_trove_set_flags(pjs->rel_entries, partitionwise_partial_match, flags);
- flags |= PGPA_TE_MATCH_FULL;
+ flags |= PGPA_FB_MATCH_FULL;
pgpa_trove_set_flags(pjs->rel_entries, partitionwise_full_match, flags);
/*
@@ -1082,7 +1081,7 @@ pgpa_planner_apply_join_path_advice(JoinType jointype, uint64 *pgs_mask_p,
* This doesn't seem to be a semijoin to which SJ_UNIQUE
* or SJ_NON_UNIQUE can be applied.
*/
- entry->flags |= PGPA_TE_INAPPLICABLE;
+ entry->flags |= PGPA_FB_INAPPLICABLE;
}
else if (advice_unique != jt_unique)
sj_deny_indexes = bms_add_member(sj_deny_indexes, i);
@@ -1102,11 +1101,11 @@ pgpa_planner_apply_join_path_advice(JoinType jointype, uint64 *pgs_mask_p,
(jo_deny_indexes != NULL || jo_deny_rel_indexes != NULL))
{
pgpa_trove_set_flags(pjs->join_entries, jo_permit_indexes,
- PGPA_TE_CONFLICTING);
+ PGPA_FB_CONFLICTING);
pgpa_trove_set_flags(pjs->join_entries, jo_deny_indexes,
- PGPA_TE_CONFLICTING);
+ PGPA_FB_CONFLICTING);
pgpa_trove_set_flags(pjs->rel_entries, jo_deny_rel_indexes,
- PGPA_TE_CONFLICTING);
+ PGPA_FB_CONFLICTING);
}
/*
@@ -1115,15 +1114,15 @@ pgpa_planner_apply_join_path_advice(JoinType jointype, uint64 *pgs_mask_p,
*/
if (jm_conflict)
pgpa_trove_set_flags(pjs->join_entries, jm_indexes,
- PGPA_TE_CONFLICTING);
+ PGPA_FB_CONFLICTING);
/* If semijoin advice says both yes and no, mark it all as conflicting. */
if (sj_permit_indexes != NULL && sj_deny_indexes != NULL)
{
pgpa_trove_set_flags(pjs->join_entries, sj_permit_indexes,
- PGPA_TE_CONFLICTING);
+ PGPA_FB_CONFLICTING);
pgpa_trove_set_flags(pjs->join_entries, sj_deny_indexes,
- PGPA_TE_CONFLICTING);
+ PGPA_FB_CONFLICTING);
}
/*
@@ -1200,7 +1199,7 @@ pgpa_join_order_permits_join(int outer_count, int inner_count,
pgpa_advice_target *prefix_target;
/* We definitely have at least a partial match for this trove entry. */
- entry->flags |= PGPA_TE_MATCH_PARTIAL;
+ entry->flags |= PGPA_FB_MATCH_PARTIAL;
/*
* Find the innermost sublist that contains all keys; if no sublist does,
@@ -1308,7 +1307,7 @@ pgpa_join_order_permits_join(int outer_count, int inner_count,
* answer is yes.
*/
if (!sublist && outer_length + 1 == length && itm == PGPA_ITM_EQUAL)
- entry->flags |= PGPA_TE_MATCH_FULL;
+ entry->flags |= PGPA_FB_MATCH_FULL;
return (itm == PGPA_ITM_EQUAL) ? PGPA_JO_PERMITTED : PGPA_JO_DENIED;
}
@@ -1334,7 +1333,7 @@ pgpa_join_order_permits_join(int outer_count, int inner_count,
* joining t1-t2 to the result would still be rejected.
*/
if (!sublist)
- entry->flags |= PGPA_TE_MATCH_FULL;
+ entry->flags |= PGPA_FB_MATCH_FULL;
return sublist ? PGPA_JO_DENIED : PGPA_JO_PERMITTED;
}
@@ -1365,7 +1364,7 @@ pgpa_join_method_permits_join(int outer_count, int inner_count,
pgpa_itm_type join_itm;
/* We definitely have at least a partial match for this trove entry. */
- entry->flags |= PGPA_TE_MATCH_PARTIAL;
+ entry->flags |= PGPA_FB_MATCH_PARTIAL;
*restrict_method = false;
@@ -1382,7 +1381,7 @@ pgpa_join_method_permits_join(int outer_count, int inner_count,
target);
if (inner_itm == PGPA_ITM_EQUAL)
{
- entry->flags |= PGPA_TE_MATCH_FULL;
+ entry->flags |= PGPA_FB_MATCH_FULL;
*restrict_method = true;
return true;
}
@@ -1469,7 +1468,7 @@ pgpa_opaque_join_permits_join(int outer_count, int inner_count,
pgpa_itm_type join_itm;
/* We definitely have at least a partial match for this trove entry. */
- entry->flags |= PGPA_TE_MATCH_PARTIAL;
+ entry->flags |= PGPA_FB_MATCH_PARTIAL;
*restrict_method = false;
@@ -1481,7 +1480,7 @@ pgpa_opaque_join_permits_join(int outer_count, int inner_count,
* We have an exact match, and should therefore allow the join and
* enforce the use of the relevant opaque join method.
*/
- entry->flags |= PGPA_TE_MATCH_FULL;
+ entry->flags |= PGPA_FB_MATCH_FULL;
*restrict_method = true;
return true;
}
@@ -1539,7 +1538,7 @@ pgpa_semijoin_permits_join(int outer_count, int inner_count,
*restrict_method = false;
/* We definitely have at least a partial match for this trove entry. */
- entry->flags |= PGPA_TE_MATCH_PARTIAL;
+ entry->flags |= PGPA_FB_MATCH_PARTIAL;
/*
* If outer rel is the nullable side and contains exactly the same
@@ -1555,7 +1554,7 @@ pgpa_semijoin_permits_join(int outer_count, int inner_count,
rids, target);
if (outer_itm == PGPA_ITM_EQUAL)
{
- entry->flags |= PGPA_TE_MATCH_FULL;
+ entry->flags |= PGPA_FB_MATCH_FULL;
if (outer_is_nullable)
{
*restrict_method = true;
@@ -1571,7 +1570,7 @@ pgpa_semijoin_permits_join(int outer_count, int inner_count,
target);
if (inner_itm == PGPA_ITM_EQUAL)
{
- entry->flags |= PGPA_TE_MATCH_FULL;
+ entry->flags |= PGPA_FB_MATCH_FULL;
if (!outer_is_nullable)
{
*restrict_method = true;
@@ -1798,7 +1797,7 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel,
/* Mark advice as inapplicable. */
pgpa_trove_set_flags(scan_entries, scan_type_indexes,
- PGPA_TE_INAPPLICABLE);
+ PGPA_FB_INAPPLICABLE);
}
else
{
@@ -1815,9 +1814,9 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel,
* Mark all the scan method entries as fully matched; and if they specify
* different things, mark them all as conflicting.
*/
- flags = PGPA_TE_MATCH_PARTIAL | PGPA_TE_MATCH_FULL;
+ flags = PGPA_FB_MATCH_PARTIAL | PGPA_FB_MATCH_FULL;
if (scan_type_conflict)
- flags |= PGPA_TE_CONFLICTING;
+ flags |= PGPA_FB_CONFLICTING;
pgpa_trove_set_flags(scan_entries, scan_type_indexes, flags);
pgpa_trove_set_flags(rel_entries, scan_type_rel_indexes, flags);
@@ -1826,11 +1825,11 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel,
* the ones that included this relation as a target by itself as fully
* matched. If there was a conflict, mark them all as conflicting.
*/
- flags = PGPA_TE_MATCH_PARTIAL;
+ flags = PGPA_FB_MATCH_PARTIAL;
if (gather_conflict)
- flags |= PGPA_TE_CONFLICTING;
+ flags |= PGPA_FB_CONFLICTING;
pgpa_trove_set_flags(rel_entries, gather_partial_match, flags);
- flags |= PGPA_TE_MATCH_FULL;
+ flags |= PGPA_FB_MATCH_FULL;
pgpa_trove_set_flags(rel_entries, gather_full_match, flags);
/*
@@ -1856,7 +1855,7 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel,
*
* Feedback entries are generated from the trove entry's flags. It's assumed
* that the caller has already set all relevant flags with the exception of
- * PGPA_TE_FAILED. We set that flag here if appropriate.
+ * PGPA_FB_FAILED. We set that flag here if appropriate.
*/
static List *
pgpa_planner_append_feedback(List *list, pgpa_trove *trove,
@@ -1878,10 +1877,10 @@ pgpa_planner_append_feedback(List *list, pgpa_trove *trove,
* from this plan would produce such an entry. If not, label the entry
* as failed.
*/
- if ((entry->flags & PGPA_TE_MATCH_FULL) != 0 &&
+ if ((entry->flags & PGPA_FB_MATCH_FULL) != 0 &&
!pgpa_walker_would_advise(walker, rt_identifiers,
entry->tag, entry->target))
- entry->flags |= PGPA_TE_FAILED;
+ entry->flags |= PGPA_FB_FAILED;
item = makeDefElem(pgpa_cstring_trove_entry(entry),
(Node *) makeInteger(entry->flags), -1);
@@ -1895,7 +1894,7 @@ pgpa_planner_append_feedback(List *list, pgpa_trove *trove,
* Emit a WARNING to tell the user about a problem with the supplied plan
* advice.
*/
-static void
+void
pgpa_planner_feedback_warning(List *feedback)
{
StringInfoData detailbuf;
@@ -1920,7 +1919,7 @@ pgpa_planner_feedback_warning(List *feedback)
* NB: Feedback should never be marked fully matched without also
* being marked partially matched.
*/
- if (flags == (PGPA_TE_MATCH_PARTIAL | PGPA_TE_MATCH_FULL))
+ if (flags == (PGPA_FB_MATCH_PARTIAL | PGPA_FB_MATCH_FULL))
continue;
/*
diff --git a/contrib/pg_plan_advice/pgpa_planner.h b/contrib/pg_plan_advice/pgpa_planner.h
index 32808b26594..366142a0c92 100644
--- a/contrib/pg_plan_advice/pgpa_planner.h
+++ b/contrib/pg_plan_advice/pgpa_planner.h
@@ -76,4 +76,7 @@ typedef struct pgpa_planner_info
*/
extern int pgpa_planner_generate_advice;
+/* Must be exported for use by test_plan_advice */
+extern PGDLLEXPORT void pgpa_planner_feedback_warning(List *feedback);
+
#endif
diff --git a/contrib/pg_plan_advice/pgpa_trove.c b/contrib/pg_plan_advice/pgpa_trove.c
index 7ade0b5ca9c..05a73cb84dd 100644
--- a/contrib/pg_plan_advice/pgpa_trove.c
+++ b/contrib/pg_plan_advice/pgpa_trove.c
@@ -23,6 +23,7 @@
*/
#include "postgres.h"
+#include "pg_plan_advice.h"
#include "pgpa_trove.h"
#include "common/hashfn_unstable.h"
@@ -321,7 +322,7 @@ pgpa_cstring_trove_entry(pgpa_trove_entry *entry)
}
/*
- * Set PGPA_TE_* flags on a set of trove entries.
+ * Set PGPA_FB_* flags on a set of trove entries.
*/
void
pgpa_trove_set_flags(pgpa_trove_entry *entries, Bitmapset *indexes, int flags)
@@ -337,26 +338,26 @@ pgpa_trove_set_flags(pgpa_trove_entry *entries, Bitmapset *indexes, int flags)
}
/*
- * Append a string representation of the specified PGPA_TE_* flags to the
+ * Append a string representation of the specified PGPA_FB_* flags to the
* given StringInfo.
*/
void
pgpa_trove_append_flags(StringInfo buf, int flags)
{
- if ((flags & PGPA_TE_MATCH_FULL) != 0)
+ if ((flags & PGPA_FB_MATCH_FULL) != 0)
{
- Assert((flags & PGPA_TE_MATCH_PARTIAL) != 0);
+ Assert((flags & PGPA_FB_MATCH_PARTIAL) != 0);
appendStringInfo(buf, "matched");
}
- else if ((flags & PGPA_TE_MATCH_PARTIAL) != 0)
+ else if ((flags & PGPA_FB_MATCH_PARTIAL) != 0)
appendStringInfo(buf, "partially matched");
else
appendStringInfo(buf, "not matched");
- if ((flags & PGPA_TE_INAPPLICABLE) != 0)
+ if ((flags & PGPA_FB_INAPPLICABLE) != 0)
appendStringInfo(buf, ", inapplicable");
- if ((flags & PGPA_TE_CONFLICTING) != 0)
+ if ((flags & PGPA_FB_CONFLICTING) != 0)
appendStringInfo(buf, ", conflicting");
- if ((flags & PGPA_TE_FAILED) != 0)
+ if ((flags & PGPA_FB_FAILED) != 0)
appendStringInfo(buf, ", failed");
}
diff --git a/contrib/pg_plan_advice/pgpa_trove.h b/contrib/pg_plan_advice/pgpa_trove.h
index 22fe3a620f7..f3afc96d666 100644
--- a/contrib/pg_plan_advice/pgpa_trove.h
+++ b/contrib/pg_plan_advice/pgpa_trove.h
@@ -19,36 +19,6 @@
typedef struct pgpa_trove pgpa_trove;
-/*
- * Flags that can be set on a pgpa_trove_entry to indicate what happened when
- * trying to plan using advice.
- *
- * PGPA_TE_MATCH_PARTIAL means that we found some part of the query that at
- * least partially matched the target; e.g. given JOIN_ORDER(a b), this would
- * be set if we ever saw any joinrel including either "a" or "b".
- *
- * PGPA_TE_MATCH_FULL means that we found an exact match for the target; e.g.
- * given JOIN_ORDER(a b), this would be set if we saw a joinrel containing
- * exactly "a" and "b" and nothing else.
- *
- * PGPA_TE_INAPPLICABLE means that the advice doesn't properly apply to the
- * target; e.g. INDEX_SCAN(foo bar_idx) would be so marked if bar_idx does not
- * exist on foo. The fact that this bit has been set does not mean that the
- * advice had no effect.
- *
- * PGPA_TE_CONFLICTING means that a conflict was detected between what this
- * advice wants and what some other plan advice wants; e.g. JOIN_ORDER(a b)
- * would conflict with HASH_JOIN(a), because the former requires "a" to be the
- * outer table while the latter requires it to be the inner table.
- *
- * PGPA_TE_FAILED means that the resulting plan did not conform to the advice.
- */
-#define PGPA_TE_MATCH_PARTIAL 0x0001
-#define PGPA_TE_MATCH_FULL 0x0002
-#define PGPA_TE_INAPPLICABLE 0x0004
-#define PGPA_TE_CONFLICTING 0x0008
-#define PGPA_TE_FAILED 0x0010
-
/*
* Each entry in a trove of advice represents the application of a tag to
* a single target.
--
2.51.0
[application/octet-stream] v26-0005-pg_plan_advice-Fix-a-bug-when-a-subquery-is-prun.patch (4.2K, 3-v26-0005-pg_plan_advice-Fix-a-bug-when-a-subquery-is-prun.patch)
download | inline diff:
From 9350af26a88bc82b38eaf63fbea4ceaf80fd8690 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Tue, 7 Apr 2026 16:56:33 -0400
Subject: [PATCH v26 5/5] pg_plan_advice: Fix a bug when a subquery is pruned
away entirely.
If a subquery is proven empty, and if that subquery contained a
semijoin, and if making one side or the other of that semijoin
unique and performing an inner join was a possible strategy, then
the previous code would fail with ERROR: no rtoffset for plan %s
when attempting to generate advice. Fix that.
Reported-by: Alexander Lakhin <[email protected]>
---
contrib/pg_plan_advice/expected/semijoin.out | 17 ++++++++++++
contrib/pg_plan_advice/pgpa_planner.c | 28 +++++++++++---------
contrib/pg_plan_advice/sql/semijoin.sql | 9 +++++++
3 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/contrib/pg_plan_advice/expected/semijoin.out b/contrib/pg_plan_advice/expected/semijoin.out
index 5551c028a1f..680de215117 100644
--- a/contrib/pg_plan_advice/expected/semijoin.out
+++ b/contrib/pg_plan_advice/expected/semijoin.out
@@ -375,3 +375,20 @@ SELECT * FROM generate_series(1,1000) g, sj_narrow s WHERE g = s.val1;
(13 rows)
COMMIT;
+-- Test the case where the subquery containing a semijoin is removed from
+-- the query entirely; this test is just to make sure that advice generation
+-- does not fail.
+EXPLAIN (COSTS OFF, PLAN_ADVICE)
+SELECT * FROM
+ (SELECT * FROM sj_narrow WHERE id IN (SELECT val1 FROM sj_wide)
+ LIMIT 1) x,
+ LATERAL (SELECT 1 WHERE false) y;
+ QUERY PLAN
+--------------------------
+ Result
+ Replaces: Scan on x
+ One-Time Filter: false
+ Generated Plan Advice:
+ NO_GATHER(x)
+(5 rows)
+
diff --git a/contrib/pg_plan_advice/pgpa_planner.c b/contrib/pg_plan_advice/pgpa_planner.c
index 86b37e79b57..72ef3230abc 100644
--- a/contrib/pg_plan_advice/pgpa_planner.c
+++ b/contrib/pg_plan_advice/pgpa_planner.c
@@ -2065,6 +2065,9 @@ pgpa_compute_rt_identifier(pgpa_planner_info *proot, PlannerInfo *root,
/*
* Compute the range table offset for each pgpa_planner_info for which it
* is possible to meaningfully do so.
+ *
+ * For pgpa_planner_info objects for which no RT offset can be computed,
+ * clear sj_unique_rels, which is meaningless in such cases.
*/
static void
pgpa_compute_rt_offsets(pgpa_planner_state *pps, PlannedStmt *pstmt)
@@ -2096,23 +2099,24 @@ pgpa_compute_rt_offsets(pgpa_planner_state *pps, PlannedStmt *pstmt)
* there's no fixed rtoffset that we can apply to the RTIs
* used during planning to locate the corresponding relations.
*/
- if (rtinfo->dummy)
+ if (!rtinfo->dummy)
{
- /*
- * It will not be possible to make any effective use of
- * the sj_unique_rels list in this case, and it also won't
- * be important to do so. So just throw the list away to
- * avoid confusing pgpa_plan_walker.
- */
- proot->sj_unique_rels = NIL;
- break;
+ Assert(!proot->has_rtoffset);
+ proot->has_rtoffset = true;
+ proot->rtoffset = rtinfo->rtoffset;
}
- Assert(!proot->has_rtoffset);
- proot->has_rtoffset = true;
- proot->rtoffset = rtinfo->rtoffset;
break;
}
}
+
+ /*
+ * If we didn't end up setting has_rtoffset, then it will not be
+ * possible to make any effective use of sj_unique_rels, and it also
+ * won't be important to do so. So just throw the list away to avoid
+ * confusing pgpa_plan_walker.
+ */
+ if (!proot->has_rtoffset)
+ proot->sj_unique_rels = NIL;
}
}
diff --git a/contrib/pg_plan_advice/sql/semijoin.sql b/contrib/pg_plan_advice/sql/semijoin.sql
index 5a4ae52d1d9..873f0d3766c 100644
--- a/contrib/pg_plan_advice/sql/semijoin.sql
+++ b/contrib/pg_plan_advice/sql/semijoin.sql
@@ -116,3 +116,12 @@ SET LOCAL pg_plan_advice.advice = 'semijoin_unique(g)';
EXPLAIN (COSTS OFF, PLAN_ADVICE)
SELECT * FROM generate_series(1,1000) g, sj_narrow s WHERE g = s.val1;
COMMIT;
+
+-- Test the case where the subquery containing a semijoin is removed from
+-- the query entirely; this test is just to make sure that advice generation
+-- does not fail.
+EXPLAIN (COSTS OFF, PLAN_ADVICE)
+SELECT * FROM
+ (SELECT * FROM sj_narrow WHERE id IN (SELECT val1 FROM sj_wide)
+ LIMIT 1) x,
+ LATERAL (SELECT 1 WHERE false) y;
--
2.51.0
[application/octet-stream] v26-0002-test_plan_advice-Guard-against-advice-instabilit.patch (11.5K, 4-v26-0002-test_plan_advice-Guard-against-advice-instabilit.patch)
download | inline diff:
From 603c2e3d20982bb0655c421d6ebe5f227af576cf Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Tue, 7 Apr 2026 13:46:04 -0400
Subject: [PATCH v26 2/5] test_plan_advice: Guard against advice instability by
replanning.
It turns out that our main regression test suite queries tables upon
which concurrent DDL is occurring, which can, rarely, cause
test_plan_advice failures. For example, if test_plan_advice plans
the query and generates INDEX_SCAN(a b) advice, and then the index
is dropped before the query is replanned with that as the supplied
advice, the advice will not apply cleanly and the tests will fail.
Such failures are apparently quite rare, but they do occur.
In an attempt to reduce the failure rate to something negligible,
this commit makes test_plan_advice drive the main planner in a
loop. We plan once, generating advice; then again, applying that
advice. If the advice doesn't apply cleanly, we do the whole thing
over again from the top. If it fails in the same way twice (same
advice, same feedback) or if we run out of retries, we emit a
warning and stop. Problems that change or vanish on retry are
assumed to be ephemeral.
Reported-by: Tom Lane <[email protected]>
---
.../test_plan_advice/t/001_replan_regress.pl | 2 +-
.../test_plan_advice/test_plan_advice.c | 233 +++++++++++++++---
2 files changed, 204 insertions(+), 31 deletions(-)
diff --git a/src/test/modules/test_plan_advice/t/001_replan_regress.pl b/src/test/modules/test_plan_advice/t/001_replan_regress.pl
index 38ffa4d11ae..e43b80bc85e 100644
--- a/src/test/modules/test_plan_advice/t/001_replan_regress.pl
+++ b/src/test/modules/test_plan_advice/t/001_replan_regress.pl
@@ -19,7 +19,7 @@ $node->init();
$node->append_conf('postgresql.conf', <<EOM);
shared_preload_libraries='test_plan_advice'
pg_plan_advice.always_explain_supplied_advice=false
-pg_plan_advice.feedback_warnings=true
+test_plan_advice.max_attempts=3
EOM
$node->start;
diff --git a/src/test/modules/test_plan_advice/test_plan_advice.c b/src/test/modules/test_plan_advice/test_plan_advice.c
index cff5039b5c8..c17115707b1 100644
--- a/src/test/modules/test_plan_advice/test_plan_advice.c
+++ b/src/test/modules/test_plan_advice/test_plan_advice.c
@@ -19,20 +19,38 @@
#include "postgres.h"
#include "access/xact.h"
+#include "commands/defrem.h"
#include "fmgr.h"
#include "optimizer/optimizer.h"
+#include "optimizer/planner.h"
#include "pg_plan_advice.h"
#include "utils/guc.h"
PG_MODULE_MAGIC;
+static int test_plan_advice_max_attempts = 1;
static bool in_recursion = false;
+static void (*feedback_warning_fn) (List *feedback);
+static planner_hook_type prev_planner_hook = NULL;
+static PlannedStmt *test_plan_advice_planner(Query *parse,
+ const char *query_string,
+ int cursorOptions,
+ ParamListInfo boundParams,
+ ExplainState *es);
static char *test_plan_advice_advisor(PlannerGlobal *glob,
Query *parse,
const char *query_string,
int cursorOptions,
ExplainState *es);
+static PlannedStmt *copy_and_plan_query(Query *parse,
+ const char *query_string,
+ int cursorOptions,
+ ParamListInfo boundParams,
+ ExplainState *es,
+ bool suppress_messages);
+static List *extract_feedback(PlannedStmt *pstmt);
+static bool all_feedback_is_ok(List *feedback);
static DefElem *find_defelem_by_defname(List *deflist, char *defname);
/*
@@ -43,6 +61,21 @@ _PG_init(void)
{
void (*add_advisor_fn) (pg_plan_advice_advisor_hook hook);
+ DefineCustomIntVariable("test_plan_advice.max_attempts",
+ "Maximum number of planning attempts before "
+ "reporting feedback warnings.",
+ NULL,
+ &test_plan_advice_max_attempts,
+ 1, 1, 10,
+ PGC_USERSET,
+ 0, NULL, NULL, NULL);
+
+ MarkGUCPrefixReserved("test_plan_advice");
+
+ /* Install our planner hook. */
+ prev_planner_hook = planner_hook;
+ planner_hook = test_plan_advice_planner;
+
/*
* Ask pg_plan_advice to get advice strings from test_plan_advice_advisor
*/
@@ -51,6 +84,78 @@ _PG_init(void)
true, NULL);
(*add_advisor_fn) (test_plan_advice_advisor);
+
+ /*
+ * Get a pointer to pg_plan_advice's function for emitting feedback
+ * warnings.
+ */
+ feedback_warning_fn =
+ load_external_function("pg_plan_advice",
+ "pgpa_planner_feedback_warning",
+ true, NULL);
+}
+
+/*
+ * Planner hook that retries planning when feedback indicates a problem.
+ *
+ * When the catalog changes between the first plan (which generates advice)
+ * and the second plan (which uses that advice), the advice can reference
+ * objects that no longer exist or reflect stale statistics. To avoid
+ * spurious warnings, we retry planning up to test_plan_advice.max_attempts
+ * times. If the feedback stabilizes (i.e. is the same as the previous
+ * attempt), we conclude the problem is genuine and emit warnings.
+ */
+static PlannedStmt *
+test_plan_advice_planner(Query *parse, const char *query_string,
+ int cursorOptions, ParamListInfo boundParams,
+ ExplainState *es)
+{
+ PlannedStmt *pstmt;
+ List *feedback;
+ List *prev_feedback = NIL;
+
+ for (int i = 0; i < test_plan_advice_max_attempts; ++i)
+ {
+ /*
+ * Try planning the query. On the first iteration, we don't need or
+ * want to suppress any warnings or other chatter that the planner is
+ * going to generate, because our goal here is to get the same output
+ * that would have occurred without this module. But on the second and
+ * later iterations, that output has already been produced, so we
+ * don't want it to appear again.
+ */
+ pstmt = copy_and_plan_query(parse, query_string, cursorOptions,
+ boundParams, es, (i > 0));
+
+ /* Extract feedback. */
+ feedback = extract_feedback(pstmt);
+
+ /* If no problems were detected, stop. */
+ if (all_feedback_is_ok(feedback))
+ break;
+
+ /*
+ * If the feedback is the same as last time, then apparently there's
+ * a real problem, so emit warnings and stop. If this is the last
+ * iteration, it's less clear that there's a real problem, but if not,
+ * the user hasn't set the maximum number of retries high enough, so
+ * handle that case the same way.
+ */
+ if (equal(feedback, prev_feedback) ||
+ i == test_plan_advice_max_attempts - 1)
+ {
+ (*feedback_warning_fn) (feedback);
+ break;
+ }
+
+ /*
+ * Go around and try it again, with the newly-generated feedback as
+ * the new point of comparison.
+ */
+ prev_feedback = feedback;
+ }
+
+ return pstmt;
}
/*
@@ -63,7 +168,6 @@ test_plan_advice_advisor(PlannerGlobal *glob, Query *parse,
ExplainState *es)
{
PlannedStmt *pstmt;
- int save_nestlevel = 0;
DefElem *pgpa_item;
DefElem *advice_string_item;
@@ -74,35 +178,18 @@ test_plan_advice_advisor(PlannerGlobal *glob, Query *parse,
if (in_recursion)
return NULL;
+ /*
+ * Try planning the query, generating advice in the process. We ask
+ * copy_and_plan_query to adjust client_min_messages; otherwise, any
+ * messages that are generated during planning would appear here and again
+ * when the query is replanned with the advice string.
+ */
PG_TRY();
{
in_recursion = true;
- /*
- * Planning can trigger expression evaluation, which can result in
- * sending NOTICE messages or other output to the client. To avoid
- * that, we set client_min_messages = ERROR in the hopes of getting
- * the same output with and without this module.
- *
- * We also need to set pg_plan_advice.always_store_advice_details so
- * that pg_plan_advice will generate an advice string, since the whole
- * point of this function is to get access to that.
- */
- save_nestlevel = NewGUCNestLevel();
- set_config_option("client_min_messages", "error",
- PGC_SUSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0, false);
- set_config_option("pg_plan_advice.always_store_advice_details", "true",
- PGC_SUSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0, false);
-
- /*
- * Replan. We must copy the Query, because the planner modifies it.
- * (As noted elsewhere, that's unfortunate; perhaps it will be fixed
- * some day.)
- */
- pstmt = planner(copyObject(parse), query_string, cursorOptions,
- glob->boundParams, es);
+ pstmt = copy_and_plan_query(parse, query_string, cursorOptions,
+ glob->boundParams, es, true);
}
PG_FINALLY();
{
@@ -110,10 +197,6 @@ test_plan_advice_advisor(PlannerGlobal *glob, Query *parse,
}
PG_END_TRY();
- /* Roll back any GUC changes */
- if (save_nestlevel > 0)
- AtEOXact_GUC(false, save_nestlevel);
-
/* Extract and return the advice string */
pgpa_item = find_defelem_by_defname(pstmt->extension_state,
"pg_plan_advice");
@@ -127,6 +210,96 @@ test_plan_advice_advisor(PlannerGlobal *glob, Query *parse,
return strVal(advice_string_item->arg);
}
+/*
+ * Wrapper around the main query planner.
+ */
+static PlannedStmt *
+copy_and_plan_query(Query *parse, const char *query_string, int cursorOptions,
+ ParamListInfo boundParams, ExplainState *es,
+ bool suppress_messages)
+{
+ int save_nestlevel = 0;
+ PlannedStmt *pstmt;
+
+ /*
+ * Temporarily set pg_plan_advice.always_store_advice_details. Either
+ * we're being called to generate advice, in which case setting this GUC
+ * is important to make sure that we do, or we're being called to see
+ * whether supplied advice applied properly, in which case this is needed
+ * so that pg_plan_advice will provide feedback.
+ */
+ save_nestlevel = NewGUCNestLevel();
+ set_config_option("pg_plan_advice.always_store_advice_details",
+ "true",
+ PGC_SUSET, PGC_S_SESSION,
+ GUC_ACTION_SAVE, true, 0, false);
+
+ /*
+ * Planning can trigger expression evaluation, which can result in sending
+ * NOTICE messages or other output to the client. To avoid that, we allow
+ * the caller to request client_min_messages = ERROR in the hopes of
+ * getting the same output with and without this module.
+ */
+ if (suppress_messages)
+ set_config_option("client_min_messages", "error",
+ PGC_SUSET, PGC_S_SESSION,
+ GUC_ACTION_SAVE, true, 0, false);
+
+ /*
+ * We must copy the Query, because the planner modifies it, and we intend
+ * to plan it multiple times. (As noted elsewhere, that's unfortunate;
+ * perhaps it will be fixed some day.)
+ */
+ if (prev_planner_hook)
+ pstmt = (*prev_planner_hook) (copyObject(parse), query_string,
+ cursorOptions, boundParams, es);
+ else
+ pstmt = standard_planner(copyObject(parse), query_string,
+ cursorOptions, boundParams, es);
+
+ /* Roll back any GUC changes */
+ AtEOXact_GUC(false, save_nestlevel);
+
+ /* And we're done. */
+ return pstmt;
+}
+
+/*
+ * Extract the feedback list from a PlannedStmt's extension_state.
+ * Returns NIL if no feedback is present.
+ */
+static List *
+extract_feedback(PlannedStmt *pstmt)
+{
+ DefElem *pgpa_item;
+ DefElem *feedback_item;
+
+ pgpa_item = find_defelem_by_defname(pstmt->extension_state,
+ "pg_plan_advice");
+ if (pgpa_item == NULL)
+ return NIL;
+ feedback_item = find_defelem_by_defname((List *) pgpa_item->arg,
+ "feedback");
+ if (feedback_item == NULL)
+ return NIL;
+ return (List *) feedback_item->arg;
+}
+
+/*
+ * Check whether a feedback list indicates that all advice was applied
+ * successfully.
+ */
+static bool
+all_feedback_is_ok(List *feedback)
+{
+ foreach_node(DefElem, item, feedback)
+ {
+ if (defGetInt32(item) != (PGPA_FB_MATCH_PARTIAL | PGPA_FB_MATCH_FULL))
+ return false;
+ }
+ return true;
+}
+
/*
* Search a list of DefElem objects for a given defname.
*/
--
2.51.0
[application/octet-stream] v26-0003-pg_plan_advice-Handle-non-repeatable-TABLESAMPLE.patch (8.5K, 5-v26-0003-pg_plan_advice-Handle-non-repeatable-TABLESAMPLE.patch)
download | inline diff:
From 0fce35fcb4125cab73358dd122fbbaa810aee5f5 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Tue, 7 Apr 2026 16:17:36 -0400
Subject: [PATCH v26 3/5] pg_plan_advice: Handle non-repeatable TABLESAMPLE
scans.
When a tablesample routine says that it is not repeatable across
scans, set_tablesample_rel_pathlist will (usually) materialize it,
confusing pg_plan_advice's plan walker machinery. To fix, update that
machinery to view such Material paths as essentially an extension of
the underlying scan.
Reported-by: Alexander Lakhin <[email protected]>
---
contrib/pg_plan_advice/Makefile | 2 ++
contrib/pg_plan_advice/expected/scan.out | 38 ++++++++++++++++++++++++
contrib/pg_plan_advice/pgpa_join.c | 12 +++++---
contrib/pg_plan_advice/pgpa_scan.c | 11 +++++++
contrib/pg_plan_advice/pgpa_walker.c | 29 ++++++++++++++++++
contrib/pg_plan_advice/pgpa_walker.h | 1 +
contrib/pg_plan_advice/sql/scan.sql | 12 ++++++++
7 files changed, 101 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_plan_advice/Makefile b/contrib/pg_plan_advice/Makefile
index cd478dc1a6d..cbafc50ca4c 100644
--- a/contrib/pg_plan_advice/Makefile
+++ b/contrib/pg_plan_advice/Makefile
@@ -22,6 +22,8 @@ PGFILEDESC = "pg_plan_advice - help the planner get the right plan"
REGRESS = gather join_order join_strategy partitionwise prepared \
scan semijoin syntax
+EXTRA_INSTALL = contrib/tsm_system_time
+
EXTRA_CLEAN = pgpa_parser.h pgpa_parser.c pgpa_scanner.c
ifdef USE_PGXS
diff --git a/contrib/pg_plan_advice/expected/scan.out b/contrib/pg_plan_advice/expected/scan.out
index 44ce40f33a6..f4036e4cbdd 100644
--- a/contrib/pg_plan_advice/expected/scan.out
+++ b/contrib/pg_plan_advice/expected/scan.out
@@ -770,3 +770,41 @@ SELECT * FROM (SELECT * FROM scan_table s WHERE a = 1 OFFSET 0);
(7 rows)
COMMIT;
+-- Test a non-repeatable tablesample method with a scan-level Materialize.
+CREATE EXTENSION tsm_system_time;
+CREATE TABLE scan_tsm (i int);
+EXPLAIN (COSTS OFF, PLAN_ADVICE)
+SELECT 1 FROM (SELECT i FROM scan_tsm TABLESAMPLE system_time (1000)),
+ LATERAL (SELECT i LIMIT 1);
+ QUERY PLAN
+----------------------------------------------------------------------
+ Nested Loop
+ -> Materialize
+ -> Sample Scan on scan_tsm
+ Sampling: system_time ('1000'::double precision)
+ -> Limit
+ -> Result
+ Generated Plan Advice:
+ JOIN_ORDER(scan_tsm unnamed_subquery#2)
+ NESTED_LOOP_PLAIN(unnamed_subquery#2)
+ NO_GATHER(unnamed_subquery#2 scan_tsm "*RESULT*"@unnamed_subquery)
+(10 rows)
+
+-- Same, but with the scan-level Materialize on the inner side of a join.
+EXPLAIN (COSTS OFF, PLAN_ADVICE)
+SELECT 1 FROM (SELECT 1 AS x LIMIT 1),
+ LATERAL (SELECT x FROM scan_tsm TABLESAMPLE system_time (1000));
+ QUERY PLAN
+--------------------------------------------------------------------
+ Nested Loop
+ -> Limit
+ -> Result
+ -> Materialize
+ -> Sample Scan on scan_tsm
+ Sampling: system_time ('1000'::double precision)
+ Generated Plan Advice:
+ JOIN_ORDER(unnamed_subquery scan_tsm)
+ NESTED_LOOP_PLAIN(scan_tsm)
+ NO_GATHER(unnamed_subquery scan_tsm "*RESULT*"@unnamed_subquery)
+(10 rows)
+
diff --git a/contrib/pg_plan_advice/pgpa_join.c b/contrib/pg_plan_advice/pgpa_join.c
index 4610d02356f..38e7b91ed7e 100644
--- a/contrib/pg_plan_advice/pgpa_join.c
+++ b/contrib/pg_plan_advice/pgpa_join.c
@@ -363,9 +363,11 @@ pgpa_decompose_join(pgpa_plan_walker_context *walker, Plan *plan,
/*
* The planner may have chosen to place a Material node on the
* inner side of the MergeJoin; if this is present, we record it
- * as part of the join strategy.
+ * as part of the join strategy. (However, scan-level Materialize
+ * nodes are an exception.)
*/
- if (elidedinner == NULL && IsA(innerplan, Material))
+ if (elidedinner == NULL && IsA(innerplan, Material) &&
+ !pgpa_is_scan_level_materialize(innerplan))
{
elidedinner = pgpa_descend_node(pstmt, &innerplan);
strategy = JSTRAT_MERGE_JOIN_MATERIALIZE;
@@ -390,9 +392,11 @@ pgpa_decompose_join(pgpa_plan_walker_context *walker, Plan *plan,
/*
* The planner may have chosen to place a Material or Memoize node
* on the inner side of the NestLoop; if this is present, we
- * record it as part of the join strategy.
+ * record it as part of the join strategy. (However, scan-level
+ * Materialize nodes are an exception.)
*/
- if (elidedinner == NULL && IsA(innerplan, Material))
+ if (elidedinner == NULL && IsA(innerplan, Material) &&
+ !pgpa_is_scan_level_materialize(innerplan))
{
elidedinner = pgpa_descend_node(pstmt, &innerplan);
strategy = JSTRAT_NESTED_LOOP_MATERIALIZE;
diff --git a/contrib/pg_plan_advice/pgpa_scan.c b/contrib/pg_plan_advice/pgpa_scan.c
index 0467f9b12ba..21b58a0ac42 100644
--- a/contrib/pg_plan_advice/pgpa_scan.c
+++ b/contrib/pg_plan_advice/pgpa_scan.c
@@ -120,6 +120,17 @@ pgpa_build_scan(pgpa_plan_walker_context *walker, Plan *plan,
break;
}
}
+ else if (pgpa_is_scan_level_materialize(plan))
+ {
+ /*
+ * Non-repeatable tablesample methods can be wrapped in a Materialize
+ * node that must be treated as part of the scan itself. See
+ * set_tablesample_rel_pathlist().
+ */
+ rti = pgpa_scanrelid(plan->lefttree);
+ relids = bms_make_singleton(rti);
+ strategy = PGPA_SCAN_ORDINARY;
+ }
else if ((relids = pgpa_relids(plan)) != NULL)
{
switch (nodeTag(plan))
diff --git a/contrib/pg_plan_advice/pgpa_walker.c b/contrib/pg_plan_advice/pgpa_walker.c
index e32684d2075..e49361ae266 100644
--- a/contrib/pg_plan_advice/pgpa_walker.c
+++ b/contrib/pg_plan_advice/pgpa_walker.c
@@ -16,6 +16,7 @@
#include "pgpa_scan.h"
#include "pgpa_walker.h"
+#include "access/tsmapi.h"
#include "nodes/plannodes.h"
#include "parser/parsetree.h"
#include "utils/lsyscache.h"
@@ -609,6 +610,34 @@ pgpa_scanrelid(Plan *plan)
}
}
+/*
+ * Check whether a plan node is a Material node that should be treated as
+ * a scan. Currently, this only happens when set_tablesample_rel_pathlist
+ * inserts a Material node to protect a SampleScan that uses a non-repeatable
+ * tablesample method.
+ *
+ * (Most Material nodes we're likely to encounter are actually part of the
+ * join strategy: nested loops and merge joins can choose to materialize the
+ * inner sides of the join. The cases identified here are the rare
+ * exceptions.)
+ */
+bool
+pgpa_is_scan_level_materialize(Plan *plan)
+{
+ Plan *child;
+ SampleScan *sscan;
+ TsmRoutine *tsm;
+
+ if (!IsA(plan, Material))
+ return false;
+ child = plan->lefttree;
+ if (child == NULL || !IsA(child, SampleScan))
+ return false;
+ sscan = (SampleScan *) child;
+ tsm = GetTsmRoutine(sscan->tablesample->tsmhandler);
+ return !tsm->repeatable_across_scans;
+}
+
/*
* Construct a new Bitmapset containing non-RTE_JOIN members of 'relids'.
*/
diff --git a/contrib/pg_plan_advice/pgpa_walker.h b/contrib/pg_plan_advice/pgpa_walker.h
index 47667c03374..ebe850622d3 100644
--- a/contrib/pg_plan_advice/pgpa_walker.h
+++ b/contrib/pg_plan_advice/pgpa_walker.h
@@ -114,6 +114,7 @@ extern void pgpa_add_future_feature(pgpa_plan_walker_context *walker,
extern ElidedNode *pgpa_last_elided_node(PlannedStmt *pstmt, Plan *plan);
extern Bitmapset *pgpa_relids(Plan *plan);
extern Index pgpa_scanrelid(Plan *plan);
+extern bool pgpa_is_scan_level_materialize(Plan *plan);
extern Bitmapset *pgpa_filter_out_join_relids(Bitmapset *relids, List *rtable);
extern bool pgpa_walker_would_advise(pgpa_plan_walker_context *walker,
diff --git a/contrib/pg_plan_advice/sql/scan.sql b/contrib/pg_plan_advice/sql/scan.sql
index 800ff7a4622..98bee88de91 100644
--- a/contrib/pg_plan_advice/sql/scan.sql
+++ b/contrib/pg_plan_advice/sql/scan.sql
@@ -196,3 +196,15 @@ SELECT * FROM (SELECT * FROM scan_table s WHERE a = 1 OFFSET 0) x;
EXPLAIN (COSTS OFF, PLAN_ADVICE)
SELECT * FROM (SELECT * FROM scan_table s WHERE a = 1 OFFSET 0);
COMMIT;
+
+-- Test a non-repeatable tablesample method with a scan-level Materialize.
+CREATE EXTENSION tsm_system_time;
+CREATE TABLE scan_tsm (i int);
+EXPLAIN (COSTS OFF, PLAN_ADVICE)
+SELECT 1 FROM (SELECT i FROM scan_tsm TABLESAMPLE system_time (1000)),
+ LATERAL (SELECT i LIMIT 1);
+
+-- Same, but with the scan-level Materialize on the inner side of a join.
+EXPLAIN (COSTS OFF, PLAN_ADVICE)
+SELECT 1 FROM (SELECT 1 AS x LIMIT 1),
+ LATERAL (SELECT x FROM scan_tsm TABLESAMPLE system_time (1000));
--
2.51.0
[application/octet-stream] v26-0004-pg_plan_advice-Add-alternatives-test-to-Makefile.patch (962B, 6-v26-0004-pg_plan_advice-Add-alternatives-test-to-Makefile.patch)
download | inline diff:
From ac0678546c6086acba0ebbcda559a345fabf52b7 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Tue, 7 Apr 2026 16:18:17 -0400
Subject: [PATCH v26 4/5] pg_plan_advice: Add alternatives test to Makefile.
Oversight in commit 6455e55b0da47255f332a96f005ba0dd1c7176c2.
---
contrib/pg_plan_advice/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/pg_plan_advice/Makefile b/contrib/pg_plan_advice/Makefile
index cbafc50ca4c..d016723794d 100644
--- a/contrib/pg_plan_advice/Makefile
+++ b/contrib/pg_plan_advice/Makefile
@@ -19,8 +19,8 @@ HEADERS_pg_plan_advice = pg_plan_advice.h
PGFILEDESC = "pg_plan_advice - help the planner get the right plan"
-REGRESS = gather join_order join_strategy partitionwise prepared \
- scan semijoin syntax
+REGRESS = alternatives gather join_order join_strategy partitionwise \
+ prepared scan semijoin syntax
EXTRA_INSTALL = contrib/tsm_system_time
--
2.51.0
view thread (184+ 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]
Subject: Re: pg_plan_advice
In-Reply-To: <CA+TgmobOOmmXSJz3e+cjTY-bA1+W0dqVDqzxUBEvGtW62whYGg@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