Fix testing of parallel-safety of SubPlans.

is_parallel_safe() supposed that the only relevant property of a SubPlan
was the parallel safety of the referenced subplan tree.  This is wrong:
the testexpr or args subtrees might contain parallel-unsafe stuff, as
demonstrated by the test case added here.  However, just recursing into the
subtrees fails in a different way: we'll typically find PARAM_EXEC Params
representing the subplan's output columns in the testexpr.  The previous
coding supposed that any Param must be treated as parallel-restricted, so
that a naive attempt at fixing this disabled parallel pushdown of SubPlans
altogether.  We must instead determine, for any visited Param, whether it
is one that would be computed by a surrounding SubPlan node; if so, it's
safe to push down along with the SubPlan node.

We might later be able to extend this logic to cope with Params used for
correlated subplans and other cases; but that's a task for v11 or beyond.

Tom Lane and Amit Kapila

Discussion: https://postgr.es/m/7064.1492022469@sss.pgh.pa.us

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index e196c5e2b5d83c49c8ac1d443ffbb61c42c0520e..a1dafc8e0f83f033297cccbab654d5ec8fc28dca 100644 (file)
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -93,6 +93,7 @@ typedef struct
 {
        char            max_hazard;             /* worst proparallel hazard found so far */
        char            max_interesting;        /* worst proparallel hazard of interest */
+       List       *safe_param_ids; /* PARAM_EXEC Param IDs to treat as safe */
 } max_parallel_hazard_context;
 
 static bool contain_agg_clause_walker(Node *node, void *context);
@@ -1056,6 +1057,7 @@ max_parallel_hazard(Query *parse)
 
        context.max_hazard = PROPARALLEL_SAFE;
        context.max_interesting = PROPARALLEL_UNSAFE;
+       context.safe_param_ids = NIL;
        (void) max_parallel_hazard_walker((Node *) parse, &context);
        return context.max_hazard;
 }
@@ -1084,6 +1086,7 @@ is_parallel_safe(PlannerInfo *root, Node *node)
        /* Else use max_parallel_hazard's search logic, but stop on RESTRICTED */
        context.max_hazard = PROPARALLEL_SAFE;
        context.max_interesting = PROPARALLEL_RESTRICTED;
+       context.safe_param_ids = NIL;
        return !max_parallel_hazard_walker(node, &context);
 }
 
@@ -1171,18 +1174,49 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context)
                        return true;
        }
 
-       /* We can push the subplans only if they are parallel-safe. */
+       /*
+        * Only parallel-safe SubPlans can be sent to workers.  Within the
+        * testexpr of the SubPlan, Params representing the output columns of the
+        * subplan can be treated as parallel-safe, so temporarily add their IDs
+        * to the safe_param_ids list while examining the testexpr.
+        */
        else if (IsA(node, SubPlan))
-               return !((SubPlan *) node)->parallel_safe;
+       {
+               SubPlan    *subplan = (SubPlan *) node;
+               List       *save_safe_param_ids;
+
+               if (!subplan->parallel_safe &&
+                       max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
+                       return true;
+               save_safe_param_ids = context->safe_param_ids;
+               context->safe_param_ids = list_concat(list_copy(subplan->paramIds),
+                                                                                         context->safe_param_ids);
+               if (max_parallel_hazard_walker(subplan->testexpr, context))
+                       return true;            /* no need to restore safe_param_ids */
+               context->safe_param_ids = save_safe_param_ids;
+               /* we must also check args, but no special Param treatment there */
+               if (max_parallel_hazard_walker((Node *) subplan->args, context))
+                       return true;
+               /* don't want to recurse normally, so we're done */
+               return false;
+       }
 
        /*
         * We can't pass Params to workers at the moment either, so they are also
-        * parallel-restricted.
+        * parallel-restricted, unless they are PARAM_EXEC Params listed in
+        * safe_param_ids, meaning they could be generated within the worker.
         */
        else if (IsA(node, Param))
        {
-               if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
-                       return true;
+               Param      *param = (Param *) node;
+
+               if (param->paramkind != PARAM_EXEC ||
+                       !list_member_int(context->safe_param_ids, param->paramid))
+               {
+                       if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
+                               return true;
+               }
+               return false;                   /* nothing to recurse to */
        }
 
        /*

diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index b87fe845458edc39a5d2ea63620d7e12304b9393..86ec82eaaae8637918b3d1a1df84ed20978b8192 100644 (file)
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -699,7 +699,8 @@ typedef struct SubPlan
        bool            unknownEqFalse; /* TRUE if it's okay to return FALSE when the
                                                                 * spec result is UNKNOWN; this allows much
                                                                 * simpler handling of null values */
-       bool            parallel_safe;  /* OK to use as part of parallel plan? */
+       bool            parallel_safe;  /* is the subplan parallel-safe? */
+       /* Note: parallel_safe does not consider contents of testexpr or args */
        /* Information for passing params into and out of the subselect: */
        /* setParam and parParam are lists of integers (param IDs) */
        List       *setParam;           /* initplan subqueries have to set these

diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 0e9bc1a70784d320a5eec62e9af9578249fc1f09..3e35e96c4b3a8f88a03fedaee9783bee224d5776 100644 (file)
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -126,6 +126,18 @@ select count(*) from tenk1 where (two, four) not in
  10000
 (1 row)
 
+-- this is not parallel-safe due to use of random() within SubLink's testexpr:
+explain (costs off)
+       select * from tenk1 where (unique1 + random())::integer not in
+       (select ten from tenk2);
+             QUERY PLAN             
+------------------------------------
+ Seq Scan on tenk1
+   Filter: (NOT (hashed SubPlan 1))
+   SubPlan 1
+     ->  Seq Scan on tenk2
+(4 rows)
+
 alter table tenk2 reset (parallel_workers);
 -- test parallel index scans.
 set enable_seqscan to off;

diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 67bc82e83477df1ab5194b8c2972d94ac2429801..d2d262c7249599cd10156f3402d0983f990dfdc9 100644 (file)
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -46,6 +46,10 @@ explain (costs off)
        (select hundred, thousand from tenk2 where thousand > 100);
 select count(*) from tenk1 where (two, four) not in
        (select hundred, thousand from tenk2 where thousand > 100);
+-- this is not parallel-safe due to use of random() within SubLink's testexpr:
+explain (costs off)
+       select * from tenk1 where (unique1 + random())::integer not in
+       (select ten from tenk2);
 alter table tenk2 reset (parallel_workers);
 
 -- test parallel index scans.