pg_plan_advice: Export feedback-related definitions.

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. We're not quite ready to fix that problem
just yet, because we want to gather some more information about how
often it actually happens first. But, our plan is going to require
test_plan_advice to access a few bits of pg_plan_advice that have
been considered internal up until now, so this commit rejiggers
things to expose those bits.

First, test_plan_advice is going to need to be able to interpret
the PGPA_TE_* constants which have been declared in pgpa_trove.h.
The "TE" stands for "trove entry" but that's kind of a silly name;
change the naming to "FB" (for "feedback") and move the declarations
to pg_plan_advice.h, which is a header file that's already installed.
This has the side benefit of making these constants available to any
other extensions that may want to examine plan advice feedback.

Second, test_plan_advice is going to call pgpa_planner_feedback_warning,
so make that function non-static and mark it PGDLLEXPORT.

Discussion: http://postgr.es/m/CA+TgmobOOmmXSJz3e+cjTY-bA1+W0dqVDqzxUBEvGtW62whYGg@mail.gmail.com

diff --git a/contrib/pg_plan_advice/pg_plan_advice.h b/contrib/pg_plan_advice/pg_plan_advice.h
index 749331b6b8a9e08b698fe5428ce4f49c147c90ec..d78477153508f0fdd2ceb5267703fd9c48a8be14 100644 (file)
--- 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 469a08424277c2e803b291e3ea2a26e2040bbe3f..72ef3230abc418550395e068af357bbef7c7e739 100644 (file)
--- 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 32808b265942e7ae603d1832591ff4f70f3676cf..366142a0c92efafd787bed495121460359f09dd6 100644 (file)
--- 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 486a42e8cc06bb1492b33b1dceaac0cc39395c3a..ca69f3bd3df6eb2d984680cb83899ba167617206 100644 (file)
--- 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);
                appendStringInfoString(buf, "matched");
        }
-       else if ((flags & PGPA_TE_MATCH_PARTIAL) != 0)
+       else if ((flags & PGPA_FB_MATCH_PARTIAL) != 0)
                appendStringInfoString(buf, "partially matched");
        else
                appendStringInfoString(buf, "not matched");
-       if ((flags & PGPA_TE_INAPPLICABLE) != 0)
+       if ((flags & PGPA_FB_INAPPLICABLE) != 0)
                appendStringInfoString(buf, ", inapplicable");
-       if ((flags & PGPA_TE_CONFLICTING) != 0)
+       if ((flags & PGPA_FB_CONFLICTING) != 0)
                appendStringInfoString(buf, ", conflicting");
-       if ((flags & PGPA_TE_FAILED) != 0)
+       if ((flags & PGPA_FB_FAILED) != 0)
                appendStringInfoString(buf, ", failed");
 }
 

diff --git a/contrib/pg_plan_advice/pgpa_trove.h b/contrib/pg_plan_advice/pgpa_trove.h
index 22fe3a620f723989f3d9a2ee59b31d55fdf1c718..f3afc96d666c918fe2f9629d4f343fc5d3128415 100644 (file)
--- 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.