Re: BUG #16536: Segfault with partition-wise joins

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> Anyway, I can see a couple of routes to a fix:

>  Tom> (1) Change create_bitmap_and_path and create_bitmap_or_path to
>  Tom> account for parameterization honestly. This is certainly the
>  Tom> cleanest fix, but it would add some cycles, and what's probably a
>  Tom> bigger issue for back-patching is that their signatures would have
>  Tom> to change. Maybe that's okay? There's probably not a reason for
>  Tom> external code to call them, and codesearch.debian.net knows of no
>  Tom> instances of that.

>  Tom> (2) Hack up reparameterize_path_by_child so that it will descend
>  Tom> into these nodes regardless of their parameterization markers.
>  Tom> That's okay from an efficiency standpoint, since we'd already have
>  Tom> stopped at the parent BitmapHeapPath if it weren't parameterized.
>  Tom> But it seems quite ugly.

> Well the obvious compromise fix is to do 2 in the back-branches and 1 in
> head, but that may be overkill...

I realized there's another way, which is to make create_bitmap_and_path
and create_bitmap_or_path compute the correct parameterization as the
union of the outer rels required by their child paths.  Then they don't
need any additional arguments.  I had been thinking of having indxpath.c
pass down the parameterization, but it turns out that that code isn't
tracking that anyway, so it would have largely had to do it like this
anyhow.

As a further benefit, now that BitmapAndPath and BitmapOrPath
aren't special in this way, we can get rid of indxpath.c's
get_bitmap_tree_required_outer() function; it's sufficient to do
PATH_REQ_OUTER() on the top-level path node in a bitmap tree.
So the attached patch actually nets out at less code than before.

You could argue I guess that although this isn't an ABI break, it's
still an API change: now these functions are required to set the
param_info field in their output correctly.  That would only break
external code if it's building such Path structures by hand rather
than using these functions.  I devoutly hope nobody is ... though
I did have to slap down some code in indxpath.c which was doing
exactly that.

This lacks regression test case(s) as yet, but I think it's
commitable otherwise.  I'm going to try to build some test cases
that improve the coverage report in this area...

>  Tom> Another point I notice is that reparameterize_path thinks it
>  Tom> doesn't need to touch sub-structure at all when increasing the
>  Tom> parameterization of a BitmapHeapPath. Maybe that's okay but it
>  Tom> probably deserves more thought, especially since I see that the
>  Tom> case is again untested.

> Hmm. I'm not sure I fully understand the implications of what's going on
> there, but if new quals are effectively being moved into the path as a
> result of the reparameterization, then leaving the substructure alone
> would presumably mean that those new quals can only become Filter:
> clauses. But presumably, if they could be usefully indexed, then we
> would have already generated a parameterized path that included them?

We're not actually moving any new quals into the path, we're just changing
its labeling.  Given the way I've done things here, I no longer think it's
a problem.  I'd worried that we might need BitmapAnd/OrPaths to have
exactly the same required_outer as their parent BitmapHeapPath, but in
the event it should be fine when they have subset required_outer values.

            regards, tom lane

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 2a50272da6..bcb1bc6097 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -122,7 +122,6 @@ static Cost bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel,
                                 List *paths);
 static PathClauseUsage *classify_index_clause_usage(Path *path,
                                                     List **clauselist);
-static Relids get_bitmap_tree_required_outer(Path *bitmapqual);
 static void find_indexpath_quals(Path *bitmapqual, List **quals, List **preds);
 static int    find_list_position(Node *node, List **nodelist);
 static bool check_index_only(RelOptInfo *rel, IndexOptInfo *index);
@@ -357,23 +356,16 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
      */
     if (bitjoinpaths != NIL)
     {
-        List       *path_outer;
         List       *all_path_outers;
         ListCell   *lc;

-        /*
-         * path_outer holds the parameterization of each path in bitjoinpaths
-         * (to save recalculating that several times), while all_path_outers
-         * holds all distinct parameterization sets.
-         */
-        path_outer = all_path_outers = NIL;
+        /* Identify each distinct parameterization seen in bitjoinpaths */
+        all_path_outers = NIL;
         foreach(lc, bitjoinpaths)
         {
             Path       *path = (Path *) lfirst(lc);
-            Relids        required_outer;
+            Relids        required_outer = PATH_REQ_OUTER(path);

-            required_outer = get_bitmap_tree_required_outer(path);
-            path_outer = lappend(path_outer, required_outer);
             if (!bms_equal_any(required_outer, all_path_outers))
                 all_path_outers = lappend(all_path_outers, required_outer);
         }
@@ -388,16 +380,14 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
             double        loop_count;
             BitmapHeapPath *bpath;
             ListCell   *lcp;
-            ListCell   *lco;

             /* Identify all the bitmap join paths needing no more than that */
             this_path_set = NIL;
-            forboth(lcp, bitjoinpaths, lco, path_outer)
+            foreach(lcp, bitjoinpaths)
             {
                 Path       *path = (Path *) lfirst(lcp);
-                Relids        p_outers = (Relids) lfirst(lco);

-                if (bms_is_subset(p_outers, max_outers))
+                if (bms_is_subset(PATH_REQ_OUTER(path), max_outers))
                     this_path_set = lappend(this_path_set, path);
             }

@@ -411,7 +401,7 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
             bitmapqual = choose_bitmap_and(root, rel, this_path_set);

             /* And push that path into the mix */
-            required_outer = get_bitmap_tree_required_outer(bitmapqual);
+            required_outer = PATH_REQ_OUTER(bitmapqual);
             loop_count = get_loop_count(root, rel->relid, required_outer);
             bpath = create_bitmap_heap_path(root, rel, bitmapqual,
                                             required_outer, loop_count, 0);
@@ -1601,25 +1591,19 @@ path_usage_comparator(const void *a, const void *b)

 /*
  * Estimate the cost of actually executing a bitmap scan with a single
- * index path (no BitmapAnd, at least not at this level; but it could be
- * a BitmapOr).
+ * index path (which could be a BitmapAnd or BitmapOr node).
  */
 static Cost
 bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath)
 {
     BitmapHeapPath bpath;
-    Relids        required_outer;
-
-    /* Identify required outer rels, in case it's a parameterized scan */
-    required_outer = get_bitmap_tree_required_outer(ipath);

     /* Set up a dummy BitmapHeapPath */
     bpath.path.type = T_BitmapHeapPath;
     bpath.path.pathtype = T_BitmapHeapScan;
     bpath.path.parent = rel;
     bpath.path.pathtarget = rel->reltarget;
-    bpath.path.param_info = get_baserel_parampathinfo(root, rel,
-                                                      required_outer);
+    bpath.path.param_info = ipath->param_info;
     bpath.path.pathkeys = NIL;
     bpath.bitmapqual = ipath;

@@ -1628,10 +1612,13 @@ bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath)
      * Parallel bitmap heap path will be considered at later stage.
      */
     bpath.path.parallel_workers = 0;
+
+    /* Now we can do cost_bitmap_heap_scan */
     cost_bitmap_heap_scan(&bpath.path, root, rel,
                           bpath.path.param_info,
                           ipath,
-                          get_loop_count(root, rel->relid, required_outer));
+                          get_loop_count(root, rel->relid,
+                                         PATH_REQ_OUTER(ipath)));

     return bpath.path.total_cost;
 }
@@ -1643,46 +1630,15 @@ bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath)
 static Cost
 bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel, List *paths)
 {
-    BitmapAndPath apath;
-    BitmapHeapPath bpath;
-    Relids        required_outer;
-
-    /* Set up a dummy BitmapAndPath */
-    apath.path.type = T_BitmapAndPath;
-    apath.path.pathtype = T_BitmapAnd;
-    apath.path.parent = rel;
-    apath.path.pathtarget = rel->reltarget;
-    apath.path.param_info = NULL;    /* not used in bitmap trees */
-    apath.path.pathkeys = NIL;
-    apath.bitmapquals = paths;
-    cost_bitmap_and_node(&apath, root);
-
-    /* Identify required outer rels, in case it's a parameterized scan */
-    required_outer = get_bitmap_tree_required_outer((Path *) &apath);
-
-    /* Set up a dummy BitmapHeapPath */
-    bpath.path.type = T_BitmapHeapPath;
-    bpath.path.pathtype = T_BitmapHeapScan;
-    bpath.path.parent = rel;
-    bpath.path.pathtarget = rel->reltarget;
-    bpath.path.param_info = get_baserel_parampathinfo(root, rel,
-                                                      required_outer);
-    bpath.path.pathkeys = NIL;
-    bpath.bitmapqual = (Path *) &apath;
+    BitmapAndPath *apath;

     /*
-     * Check the cost of temporary path without considering parallelism.
-     * Parallel bitmap heap path will be considered at later stage.
+     * Might as well build a real BitmapAndPath here, as the work is slightly
+     * too complicated to be worth repeating just to save one palloc.
      */
-    bpath.path.parallel_workers = 0;
-
-    /* Now we can do cost_bitmap_heap_scan */
-    cost_bitmap_heap_scan(&bpath.path, root, rel,
-                          bpath.path.param_info,
-                          (Path *) &apath,
-                          get_loop_count(root, rel->relid, required_outer));
+    apath = create_bitmap_and_path(root, rel, paths);

-    return bpath.path.total_cost;
+    return bitmap_scan_cost_est(root, rel, (Path *) apath);
 }


@@ -1753,49 +1709,6 @@ classify_index_clause_usage(Path *path, List **clauselist)
 }


-/*
- * get_bitmap_tree_required_outer
- *        Find the required outer rels for a bitmap tree (index/and/or)
- *
- * We don't associate any particular parameterization with a BitmapAnd or
- * BitmapOr node; however, the IndexPaths have parameterization info, in
- * their capacity as standalone access paths.  The parameterization required
- * for the bitmap heap scan node is the union of rels referenced in the
- * child IndexPaths.
- */
-static Relids
-get_bitmap_tree_required_outer(Path *bitmapqual)
-{
-    Relids        result = NULL;
-    ListCell   *lc;
-
-    if (IsA(bitmapqual, IndexPath))
-    {
-        return bms_copy(PATH_REQ_OUTER(bitmapqual));
-    }
-    else if (IsA(bitmapqual, BitmapAndPath))
-    {
-        foreach(lc, ((BitmapAndPath *) bitmapqual)->bitmapquals)
-        {
-            result = bms_join(result,
-                              get_bitmap_tree_required_outer((Path *) lfirst(lc)));
-        }
-    }
-    else if (IsA(bitmapqual, BitmapOrPath))
-    {
-        foreach(lc, ((BitmapOrPath *) bitmapqual)->bitmapquals)
-        {
-            result = bms_join(result,
-                              get_bitmap_tree_required_outer((Path *) lfirst(lc)));
-        }
-    }
-    else
-        elog(ERROR, "unrecognized node type: %d", nodeTag(bitmapqual));
-
-    return result;
-}
-
-
 /*
  * find_indexpath_quals
  *
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index e845a4b1ae..aedb5c9dfa 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1081,11 +1081,27 @@ create_bitmap_and_path(PlannerInfo *root,
                        List *bitmapquals)
 {
     BitmapAndPath *pathnode = makeNode(BitmapAndPath);
+    Relids        required_outer = NULL;
+    ListCell   *lc;

     pathnode->path.pathtype = T_BitmapAnd;
     pathnode->path.parent = rel;
     pathnode->path.pathtarget = rel->reltarget;
-    pathnode->path.param_info = NULL;    /* not used in bitmap trees */
+
+    /*
+     * Identify the required outer rels as the union of what the child paths
+     * depend on.  (Alternatively, we could insist that the caller pass this
+     * in, but it's more convenient and reliable to compute it here.)
+     */
+    foreach(lc, bitmapquals)
+    {
+        Path       *bitmapqual = (Path *) lfirst(lc);
+
+        required_outer = bms_add_members(required_outer,
+                                         PATH_REQ_OUTER(bitmapqual));
+    }
+    pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
+                                                          required_outer);

     /*
      * Currently, a BitmapHeapPath, BitmapAndPath, or BitmapOrPath will be
@@ -1117,11 +1133,27 @@ create_bitmap_or_path(PlannerInfo *root,
                       List *bitmapquals)
 {
     BitmapOrPath *pathnode = makeNode(BitmapOrPath);
+    Relids        required_outer = NULL;
+    ListCell   *lc;

     pathnode->path.pathtype = T_BitmapOr;
     pathnode->path.parent = rel;
     pathnode->path.pathtarget = rel->reltarget;
-    pathnode->path.param_info = NULL;    /* not used in bitmap trees */
+
+    /*
+     * Identify the required outer rels as the union of what the child paths
+     * depend on.  (Alternatively, we could insist that the caller pass this
+     * in, but it's more convenient and reliable to compute it here.)
+     */
+    foreach(lc, bitmapquals)
+    {
+        Path       *bitmapqual = (Path *) lfirst(lc);
+
+        required_outer = bms_add_members(required_outer,
+                                         PATH_REQ_OUTER(bitmapqual));
+    }
+    pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
+                                                          required_outer);

     /*
      * Currently, a BitmapHeapPath, BitmapAndPath, or BitmapOrPath will be