Re: inconsistent results querying table partitioned by date

David Rowley <david.rowley@2ndquadrant.com> writes:
> [ dont_prune_with_nonimmutable_opfuncs_during_planning_v3.patch ]

I started working through this, and changed some comments I thought could
be improved, and noticed that you'd missed out making the same changes
for ScalarArrayOp so I did that.  However, when I got to the changes in
analyze_partkey_exprs, my bogometer went off.  Why do we need to recheck
this, in fact why does that function exist at all?  ISTM if we have used
a pruning qual at plan time there is no need to check it again at runtime;
therefore, it shouldn't even be in the list that analyze_partkey_exprs
is looking at.

I am wondering therefore whether we shouldn't do one of these two
things:

* Teach match_clause_to_partition_key to not emit plan-time-usable
quals at all if it is called with forplanner = false, or

* Add a bool field to PartitionPruneStep indicating whether the step
can be used at plan time, and use that to skip redundant checks at run
time.  This would make more sense if we can then fix things so that
we only run match_clause_to_partition_key once per qual clause ---
I gather that it's now being run twice, which seems pretty inefficient.

Perhaps this implies too much code churn/restructuring to be reasonable
to consider right now, but I thought I'd ask.  It sure seems like this
work is being done in a pretty inefficient and duplicative way.

            regards, tom lane

diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 59f34f5..a628858 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -39,6 +39,7 @@
 #include "access/nbtree.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_opfamily.h"
+#include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "executor/executor.h"
 #include "miscadmin.h"
@@ -93,8 +94,12 @@ typedef enum PartClauseMatchStatus
  */
 typedef struct GeneratePruningStepsContext
 {
+    /* Input data: */
+    bool        forplanner;        /* true when generating steps to be used
+                                 * during query planning */
+    /* Working state and result data: */
     int            next_step_id;
-    List       *steps;
+    List       *steps;            /* output, list of PartitionPruneSteps */
 } GeneratePruningStepsContext;

 /* The result of performing one PartitionPruneStep */
@@ -117,7 +122,7 @@ static List *make_partitionedrel_pruneinfo(PlannerInfo *root,
                               List *partitioned_rels, List *prunequal,
                               Bitmapset **matchedsubplans);
 static List *gen_partprune_steps(RelOptInfo *rel, List *clauses,
-                    bool *contradictory);
+                    bool forplanner, bool *contradictory);
 static List *gen_partprune_steps_internal(GeneratePruningStepsContext *context,
                              RelOptInfo *rel, List *clauses,
                              bool *contradictory);
@@ -409,8 +414,13 @@ make_partitionedrel_pruneinfo(PlannerInfo *root, RelOptInfo *parentrel,
                                                   targetpart->relids);
         }

-        /* Convert pruning qual to pruning steps. */
-        pruning_steps = gen_partprune_steps(subpart, partprunequal,
+        /*
+         * Convert pruning qual to pruning steps.  Since these steps will be
+         * used in the executor, we can pass 'forplanner' as false to allow
+         * steps to be generated that are unsafe for evaluation during
+         * planning, e.g. evaluation of stable functions.
+         */
+        pruning_steps = gen_partprune_steps(subpart, partprunequal, false,
                                             &contradictory);

         if (contradictory)
@@ -523,16 +533,21 @@ make_partitionedrel_pruneinfo(PlannerInfo *root, RelOptInfo *parentrel,
 /*
  * gen_partprune_steps
  *        Process 'clauses' (a rel's baserestrictinfo list of clauses) and return
- *        a list of "partition pruning steps"
+ *        a list of "partition pruning steps".
+ *
+ * 'forplanner' must be true when generating steps to be evaluated during
+ * query planning, false when generating steps to be used at run-time.
  *
  * If the clauses in the input list are contradictory or there is a
  * pseudo-constant "false", *contradictory is set to true upon return.
  */
 static List *
-gen_partprune_steps(RelOptInfo *rel, List *clauses, bool *contradictory)
+gen_partprune_steps(RelOptInfo *rel, List *clauses, bool forplanner,
+                    bool *contradictory)
 {
     GeneratePruningStepsContext context;

+    context.forplanner = forplanner;
     context.next_step_id = 0;
     context.steps = NIL;

@@ -574,9 +589,11 @@ gen_partprune_steps(RelOptInfo *rel, List *clauses, bool *contradictory)

 /*
  * prune_append_rel_partitions
- *        Returns indexes into rel->part_rels of the minimum set of child
- *        partitions which must be scanned to satisfy rel's baserestrictinfo
- *        quals.
+ *        Process rel's baserestrictinfo and make use of quals which can be
+ *        evaluated during query planning in order to determine the minimum set
+ *        of partitions which must be scanned to satisfy these quals.  Returns
+ *        the matching partitions in the form of a Bitmapset containing the
+ *        partitions' indexes in the rel's part_rels array.
  *
  * Callers must ensure that 'rel' is a partitioned table.
  */
@@ -603,9 +620,12 @@ prune_append_rel_partitions(RelOptInfo *rel)

     /*
      * Process clauses.  If the clauses are found to be contradictory, we can
-     * return the empty set.
+     * return the empty set.  Pass 'forplanner' as true to indicate to the
+     * pruning code that we only want pruning steps that can be evaluated
+     * during planning.
      */
-    pruning_steps = gen_partprune_steps(rel, clauses, &contradictory);
+    pruning_steps = gen_partprune_steps(rel, clauses, true,
+                                        &contradictory);
     if (contradictory)
         return NULL;

@@ -635,6 +655,9 @@ prune_append_rel_partitions(RelOptInfo *rel)
  * get_matching_partitions
  *        Determine partitions that survive partition pruning
  *
+ * Note: context->planstate must be set to a valid PlanState when the
+ * pruning_steps were generated with 'forplanner' = false.
+ *
  * Returns a Bitmapset of the RelOptInfo->part_rels indexes of the surviving
  * partitions.
  */
@@ -1572,8 +1595,7 @@ match_clause_to_partition_key(RelOptInfo *rel,
     Expr       *expr;

     /*
-     * Recognize specially shaped clauses that match with the Boolean
-     * partition key.
+     * Recognize specially shaped clauses that match a Boolean partition key.
      */
     if (match_boolean_partition_clause(partopfamily, clause, partkey, &expr))
     {
@@ -1648,16 +1670,47 @@ match_clause_to_partition_key(RelOptInfo *rel,
          * Matched with this key.  Now check various properties of the clause
          * to see if it's sane to use it for pruning.  In most of these cases,
          * we can return UNSUPPORTED because the same failure would occur no
-         * matter which partkey it's matched to.
+         * matter which partkey it's matched to.  (In particular, now that
+         * we've successfully matched one side of the operator to a partkey,
+         * there is no chance that matching the other side to another partkey
+         * will produce a usable result, since that'd mean there are Vars on
+         * both sides.)
          */
+        if (context->forplanner)
+        {
+            /*
+             * When pruning in the planner, we only support pruning using
+             * comparisons to constants.  Immutable subexpressions will have
+             * been folded to constants already, and we cannot prune on the
+             * basis of anything that's not immutable.
+             */
+            if (!IsA(expr, Const))
+                return PARTCLAUSE_UNSUPPORTED;

-        /*
-         * We can't prune using an expression with Vars.  (Report failure as
-         * UNSUPPORTED, not NOMATCH: as in the no-commutator case above, we
-         * now know there are Vars on both sides, so it's no good.)
-         */
-        if (contain_var_clause((Node *) expr))
-            return PARTCLAUSE_UNSUPPORTED;
+            /*
+             * Also, the comparison operator itself must be immutable.
+             */
+            if (op_volatile(opno) != PROVOLATILE_IMMUTABLE)
+                return PARTCLAUSE_UNSUPPORTED;
+        }
+        else
+        {
+            /*
+             * Otherwise, non-consts are allowed, but we can't prune using an
+             * expression that contains Vars.
+             */
+            if (contain_var_clause((Node *) expr))
+                return PARTCLAUSE_UNSUPPORTED;
+
+            /*
+             * And we must reject anything containing a volatile function.
+             * Stable functions are OK though.  (We need not check this for
+             * the comparison operator itself: anything that belongs to a
+             * partitioning operator family must be at least stable.)
+             */
+            if (contain_volatile_functions((Node *) expr))
+                return PARTCLAUSE_UNSUPPORTED;
+        }

         /*
          * Only allow strict operators.  This will guarantee nulls are
@@ -1666,10 +1719,6 @@ match_clause_to_partition_key(RelOptInfo *rel,
         if (!op_strict(opno))
             return PARTCLAUSE_UNSUPPORTED;

-        /* We can't use any volatile expressions to prune partitions. */
-        if (contain_volatile_functions((Node *) expr))
-            return PARTCLAUSE_UNSUPPORTED;
-
         /*
          * See if the operator is relevant to the partitioning opfamily.
          *
@@ -1798,20 +1847,51 @@ match_clause_to_partition_key(RelOptInfo *rel,
         if (IsA(leftop, RelabelType))
             leftop = ((RelabelType *) leftop)->arg;

-        /* Check it matches this partition key */
+        /* check if the LHS matches this partition key */
         if (!equal(leftop, partkey) ||
             !PartCollMatchesExprColl(partcoll, saop->inputcollid))
             return PARTCLAUSE_NOMATCH;

         /*
          * Matched with this key.  Check various properties of the clause to
-         * see if it can sanely be used for partition pruning (this is mostly
-         * the same as for a plain OpExpr).
+         * see if it can sanely be used for partition pruning (this is
+         * identical to the logic for a plain OpExpr).
          */
+        if (context->forplanner)
+        {
+            /*
+             * When pruning in the planner, we only support pruning using
+             * comparisons to constants.  Immutable subexpressions will have
+             * been folded to constants already, and we cannot prune on the
+             * basis of anything that's not immutable.
+             */
+            if (!IsA(rightop, Const))
+                return PARTCLAUSE_UNSUPPORTED;

-        /* We can't prune using an expression with Vars. */
-        if (contain_var_clause((Node *) rightop))
-            return PARTCLAUSE_UNSUPPORTED;
+            /*
+             * Also, the comparison operator itself must be immutable.
+             */
+            if (op_volatile(saop_op) != PROVOLATILE_IMMUTABLE)
+                return PARTCLAUSE_UNSUPPORTED;
+        }
+        else
+        {
+            /*
+             * Otherwise, non-consts are allowed, but we can't prune using an
+             * expression that contains Vars.
+             */
+            if (contain_var_clause((Node *) rightop))
+                return PARTCLAUSE_UNSUPPORTED;
+
+            /*
+             * And we must reject anything containing a volatile function.
+             * Stable functions are OK though.  (We need not check this for
+             * the comparison operator itself: anything that belongs to a
+             * partitioning operator family must be at least stable.)
+             */
+            if (contain_volatile_functions((Node *) rightop))
+                return PARTCLAUSE_UNSUPPORTED;
+        }

         /*
          * Only allow strict operators.  This will guarantee nulls are
@@ -1820,10 +1900,6 @@ match_clause_to_partition_key(RelOptInfo *rel,
         if (!op_strict(saop_op))
             return PARTCLAUSE_UNSUPPORTED;

-        /* We can't use any volatile expressions to prune partitions. */
-        if (contain_volatile_functions((Node *) rightop))
-            return PARTCLAUSE_UNSUPPORTED;
-
         /*
          * In case of NOT IN (..), we get a '<>', which we handle if list
          * partitioning is in use and we're able to confirm that it's negator
@@ -2961,15 +3037,17 @@ analyze_partkey_exprs(PartitionedRelPruneInfo *pinfo, List *steps,
     {
         PartitionPruneStepOp *step = (PartitionPruneStepOp *) lfirst(lc);
         ListCell   *lc2;
+        ListCell   *lc3;
         int            keyno;

         if (!IsA(step, PartitionPruneStepOp))
             continue;

         keyno = 0;
-        foreach(lc2, step->exprs)
+        forboth(lc2, step->exprs, lc3, step->cmpfns)
         {
             Expr       *expr = lfirst(lc2);
+            Oid            fnoid = lfirst_oid(lc3);

             if (!IsA(expr, Const))
             {
@@ -2992,6 +3070,18 @@ analyze_partkey_exprs(PartitionedRelPruneInfo *pinfo, List *steps,

                 doruntimeprune = true;
             }
+
+            /*
+             * Pruning done during planning won't have pruned using quals with
+             * a non-immutable comparison functions, so we'll switch on
+             * run-time pruning for these.
+             */
+            else if (func_volatile(fnoid) != PROVOLATILE_IMMUTABLE)
+            {
+                /* No need to check for exec params inside a const */
+                doruntimeprune = true;
+                pinfo->do_initial_prune = true;
+            }
             keyno++;
         }
     }
@@ -3347,6 +3437,12 @@ partkey_datum_from_expr(PartitionPruneContext *context,
     else
     {
         /*
+         * We should never see a non-Const in a step unless we're running in
+         * the executor.
+         */
+        Assert(context->planstate != NULL);
+
+        /*
          * When called from the executor we'll have a valid planstate so we
          * may be able to evaluate an expression which could not be folded to
          * a Const during planning.  Since run-time pruning can occur both
@@ -3354,9 +3450,7 @@ partkey_datum_from_expr(PartitionPruneContext *context,
          * must be careful here to evaluate expressions containing PARAM_EXEC
          * Params only when told it's OK.
          */
-        if (context->planstate &&
-            (context->evalexecparams ||
-             !context->exprhasexecparam[stateidx]))
+        if (context->evalexecparams || !context->exprhasexecparam[stateidx])
         {
             ExprState  *exprstate;
             ExprContext *ectx;
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index bd64bed..cad5967 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -3036,6 +3036,41 @@ select * from listp where a = (select null::int);
 (7 rows)

 drop table listp;
+--
+-- check that stable query clauses are only used in run-time pruning
+--
+create table stable_qual_pruning (a timestamp) partition by range (a);
+create table stable_qual_pruning1 partition of stable_qual_pruning
+  for values from ('2000-01-01') to ('2000-02-01');
+create table stable_qual_pruning2 partition of stable_qual_pruning
+  for values from ('2000-02-01') to ('2000-03-01');
+create table stable_qual_pruning3 partition of stable_qual_pruning
+  for values from ('3000-02-01') to ('3000-03-01');
+-- comparison against a stable value requires run-time pruning
+explain (analyze, costs off, summary off, timing off)
+select * from stable_qual_pruning where a < localtimestamp;
+                           QUERY PLAN
+----------------------------------------------------------------
+ Append (actual rows=0 loops=1)
+   Subplans Removed: 1
+   ->  Seq Scan on stable_qual_pruning1 (actual rows=0 loops=1)
+         Filter: (a < LOCALTIMESTAMP)
+   ->  Seq Scan on stable_qual_pruning2 (actual rows=0 loops=1)
+         Filter: (a < LOCALTIMESTAMP)
+(6 rows)
+
+-- timestamp < timestamptz comparison is only stable, not immutable
+explain (analyze, costs off, summary off, timing off)
+select * from stable_qual_pruning where a < '2000-02-01'::timestamptz;
+                                   QUERY PLAN
+--------------------------------------------------------------------------------
+ Append (actual rows=0 loops=1)
+   Subplans Removed: 2
+   ->  Seq Scan on stable_qual_pruning1 (actual rows=0 loops=1)
+         Filter: (a < 'Tue Feb 01 00:00:00 2000 PST'::timestamp with time zone)
+(4 rows)
+
+drop table stable_qual_pruning;
 -- Ensure runtime pruning works with initplans params with boolean types
 create table boolvalues (value bool not null);
 insert into boolvalues values('t'),('f');
diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index 246c627..94d9e1e 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -754,6 +754,27 @@ select * from listp where a = (select null::int);

 drop table listp;

+--
+-- check that stable query clauses are only used in run-time pruning
+--
+create table stable_qual_pruning (a timestamp) partition by range (a);
+create table stable_qual_pruning1 partition of stable_qual_pruning
+  for values from ('2000-01-01') to ('2000-02-01');
+create table stable_qual_pruning2 partition of stable_qual_pruning
+  for values from ('2000-02-01') to ('2000-03-01');
+create table stable_qual_pruning3 partition of stable_qual_pruning
+  for values from ('3000-02-01') to ('3000-03-01');
+
+-- comparison against a stable value requires run-time pruning
+explain (analyze, costs off, summary off, timing off)
+select * from stable_qual_pruning where a < localtimestamp;
+
+-- timestamp < timestamptz comparison is only stable, not immutable
+explain (analyze, costs off, summary off, timing off)
+select * from stable_qual_pruning where a < '2000-02-01'::timestamptz;
+
+drop table stable_qual_pruning;
+
 -- Ensure runtime pruning works with initplans params with boolean types
 create table boolvalues (value bool not null);
 insert into boolvalues values('t'),('f');