Re: Teach predtest about IS [NOT] proofs - Mailing list pgsql-hackers

James Coleman <jtc331@gmail.com> writes:
> I've not yet applied all of your feedback, but I wanted to get an
> initial read on your thoughts on how using switch statements ends up
> looking. Attached is a single (pure refactor) patch that converts the
> various if/else levels that check things like node tag and
> boolean/null test type into switch statements. I've retained 'default'
> keyword usages for now for simplicity (my intuition is that we
> generally prefer to list out all options for compiler safety benefits,
> though I'm not 100% sure that's useful in the outer node tag check
> since it's unlikely that someone adding a new node would modify
> this...).

> My big question is: are you comfortable with the indentation explosion
> this creates? IMO it's a lot wordier, but it is also more obvious what
> the structural goal is. I'm not sure how we want to make the right
> trade-off though.

Yeah, I see what you mean.  Also, I'd wanted to shove most of
the text in the function header in-line and get rid of the short
restatements of those paras.  I carried that through just for
predicate_implied_by_simple_clause, as attached.  The structure is
definitely clearer, but we end up with an awful lot of indentation,
which makes the comments less readable than I'd like.  (I did some
minor rewording to make them flow better.)

On balance I think this is probably better than what we have, but
maybe we'd be best off to avoid doubly nested switches?  I think
there's a good argument for the outer switch on nodeTag, but
maybe we're getting diminishing returns from an inner switch.

            regards, tom lane

diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c
index fe83e45311..a9accbed53 100644
--- a/src/backend/optimizer/util/predtest.c
+++ b/src/backend/optimizer/util/predtest.c
@@ -1087,38 +1087,12 @@ arrayexpr_cleanup_fn(PredIterInfo info)
 }


-/*----------
+/*
  * predicate_implied_by_simple_clause
  *      Does the predicate implication test for a "simple clause" predicate
  *      and a "simple clause" restriction.
  *
  * We return true if able to prove the implication, false if not.
- *
- * We have several strategies for determining whether one simple clause
- * implies another:
- *
- * A simple and general way is to see if they are equal(); this works for any
- * kind of expression, and for either implication definition.  (Actually,
- * there is an implied assumption that the functions in the expression are
- * immutable --- but this was checked for the predicate by the caller.)
- *
- * Another way that always works is that for boolean x, "x = TRUE" is
- * equivalent to "x", likewise "x = FALSE" is equivalent to "NOT x".
- * These can be worth checking because, while we preferentially simplify
- * boolean comparisons down to "x" and "NOT x", the other form has to be
- * dealt with anyway in the context of index conditions.
- *
- * If the predicate is of the form "foo IS NOT NULL", and we are considering
- * strong implication, we can conclude that the predicate is implied if the
- * clause is strict for "foo", i.e., it must yield false or NULL when "foo"
- * is NULL.  In that case truth of the clause ensures that "foo" isn't NULL.
- * (Again, this is a safe conclusion because "foo" must be immutable.)
- * This doesn't work for weak implication, though.
- *
- * Finally, if both clauses are binary operator expressions, we may be able
- * to prove something using the system's knowledge about operators; those
- * proof rules are encapsulated in operator_predicate_proof().
- *----------
  */
 static bool
 predicate_implied_by_simple_clause(Expr *predicate, Node *clause,
@@ -1127,65 +1101,115 @@ predicate_implied_by_simple_clause(Expr *predicate, Node *clause,
     /* Allow interrupting long proof attempts */
     CHECK_FOR_INTERRUPTS();

-    /* First try the equal() test */
+    /*
+     * A simple and general rule is that a clause implies itself, hence we
+     * check if they are equal(); this works for any kind of expression, and
+     * for either implication definition.  (Actually, there is an implied
+     * assumption that the functions in the expression are immutable --- but
+     * this was checked for the predicate by the caller.)
+     */
     if (equal((Node *) predicate, clause))
         return true;

-    /* Next see if clause is boolean equality to a constant */
-    if (is_opclause(clause) &&
-        ((OpExpr *) clause)->opno == BooleanEqualOperator)
+    /* Our remaining strategies are all clause-type-specific */
+    switch (nodeTag(clause))
     {
-        OpExpr       *op = (OpExpr *) clause;
-        Node       *rightop;
-
-        Assert(list_length(op->args) == 2);
-        rightop = lsecond(op->args);
-        /* We might never see a null Const here, but better check anyway */
-        if (rightop && IsA(rightop, Const) &&
-            !((Const *) rightop)->constisnull)
-        {
-            Node       *leftop = linitial(op->args);
-
-            if (DatumGetBool(((Const *) rightop)->constvalue))
+        case T_OpExpr:
             {
-                /* X = true implies X */
-                if (equal(predicate, leftop))
-                    return true;
+                OpExpr       *op = (OpExpr *) clause;
+
+                /*----------
+                 * For boolean x, "x = TRUE" is equivalent to "x", likewise
+                 * "x = FALSE" is equivalent to "NOT x".  These can be worth
+                 * checking because, while we preferentially simplify boolean
+                 * comparisons down to "x" and "NOT x", the other form has to
+                 * be dealt with anyway in the context of index conditions.
+                 *
+                 * We could likewise check whether the predicate is boolean
+                 * equality to a constant; but there are no known use-cases
+                 * for that at the moment, assuming that the predicate has
+                 * been through constant-folding.
+                 *----------
+                 */
+                if (op->opno == BooleanEqualOperator)
+                {
+                    Node       *rightop;
+
+                    Assert(list_length(op->args) == 2);
+                    rightop = lsecond(op->args);
+
+                    /*
+                     * We might never see a null Const here, but better check
+                     * anyway
+                     */
+                    if (rightop && IsA(rightop, Const) &&
+                        !((Const *) rightop)->constisnull)
+                    {
+                        Node       *leftop = linitial(op->args);
+
+                        if (DatumGetBool(((Const *) rightop)->constvalue))
+                        {
+                            /* X = true implies X */
+                            if (equal(predicate, leftop))
+                                return true;
+                        }
+                        else
+                        {
+                            /* X = false implies NOT X */
+                            if (is_notclause(predicate) &&
+                                equal(get_notclausearg(predicate), leftop))
+                                return true;
+                        }
+                    }
+                }
             }
-            else
+            break;
+        default:
+            break;
+    }
+
+    /* ... or predicate-type-specific */
+    switch (nodeTag(predicate))
+    {
+        case T_NullTest:
             {
-                /* X = false implies NOT X */
-                if (is_notclause(predicate) &&
-                    equal(get_notclausearg(predicate), leftop))
-                    return true;
+                NullTest   *ntest = (NullTest *) predicate;
+
+                switch (ntest->nulltesttype)
+                {
+                    case IS_NOT_NULL:
+
+                        /*
+                         * If the predicate is of the form "foo IS NOT NULL",
+                         * and we are considering strong implication, we can
+                         * conclude that the predicate is implied if the
+                         * clause is strict for "foo", i.e., it must yield
+                         * false or NULL when "foo" is NULL.  In that case
+                         * truth of the clause ensures that "foo" isn't NULL.
+                         * (Again, this is a safe conclusion because "foo"
+                         * must be immutable.)  This doesn't work for weak
+                         * implication, though.  Also, "row IS NOT NULL" does
+                         * not act in the simple way we have in mind.
+                         */
+                        if (!weak &&
+                            !ntest->argisrow &&
+                            clause_is_strict_for(clause, (Node *) ntest->arg, true))
+                            return true;
+                        break;
+                    default:
+                        break;
+                }
             }
-        }
+            break;
+        default:
+            break;
     }

     /*
-     * We could likewise check whether the predicate is boolean equality to a
-     * constant; but there are no known use-cases for that at the moment,
-     * assuming that the predicate has been through constant-folding.
+     * If both clauses are binary operator expressions, we may be able to
+     * prove something using the system's knowledge about operators; those
+     * proof rules are encapsulated in operator_predicate_proof().
      */
-
-    /* Next try the IS NOT NULL case */
-    if (!weak &&
-        predicate && IsA(predicate, NullTest))
-    {
-        NullTest   *ntest = (NullTest *) predicate;
-
-        /* row IS NOT NULL does not act in the simple way we have in mind */
-        if (ntest->nulltesttype == IS_NOT_NULL &&
-            !ntest->argisrow)
-        {
-            /* strictness of clause for foo implies foo IS NOT NULL */
-            if (clause_is_strict_for(clause, (Node *) ntest->arg, true))
-                return true;
-        }
-        return false;            /* we can't succeed below... */
-    }
-
-    /* Else try operator-related knowledge */
     return operator_predicate_proof(predicate, clause, false, weak);
 }