Fix incorrect tests for SRFs in relation_can_be_sorted_early().

Commit fac1b470a thought we could check for set-returning functions
by testing only the top-level node in an expression tree.  This is
wrong in itself, and to make matters worse it encouraged others
to make the same mistake, by exporting tlist.c's special-purpose
IS_SRF_CALL() as a widely-visible macro.  I can't find any evidence
that anyone's taken the bait, but it was only a matter of time.

Use expression_returns_set() instead, and stuff the IS_SRF_CALL()
genie back in its bottle, this time with a warning label.  I also
added a couple of cross-reference comments.

After a fair amount of fooling around, I've despaired of making
a robust test case that exposes the bug reliably, so no test case
here.  (Note that the test case added by fac1b470a is itself
broken, in that it doesn't notice if you remove the code change.
The repro given by the bug submitter currently doesn't fail either
in v15 or HEAD, though I suspect that may indicate an unrelated bug.)

Per bug #17564 from Martijn van Oosterhout.  Back-patch to v13,
as the faulty patch was.

Discussion: https://postgr.es/m/17564-c7472c2f90ef2da3@postgresql.org

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index cbf15aa48d668d8a43fbd6fcb0671321d39c8844..585b0046f4f092f8c22a247923261082e5f04b67 100644 (file)
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -742,6 +742,12 @@ expression_returns_set_walker(Node *node, void *context)
        /* else fall through to check args */
    }
 
+   /*
+    * If you add any more cases that return sets, also fix
+    * expression_returns_set_rows() in clauses.c and IS_SRF_CALL() in
+    * tlist.c.
+    */
+
    /* Avoid recursion for some cases that parser checks not to return a set */
    if (IsA(node, Aggref))
        return false;

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 7da261c03127735e2638156545652b342d25422d..159bb7e6531c7b7b64516376f6b5d1c9d9691aa1 100644 (file)
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -1002,7 +1002,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
         * one are effectively checking properties of targetexpr, so there's
         * no point in asking whether some other EC member would be better.)
         */
-       if (IS_SRF_CALL((Node *) em->em_expr))
+       if (expression_returns_set((Node *) em->em_expr))
            continue;
 
        /*
@@ -1030,7 +1030,7 @@ find_em_expr_usable_for_sorting_rel(PlannerInfo *root, EquivalenceClass *ec,
     * member in this case; since SRFs can't appear in WHERE, they cannot
     * belong to multi-member ECs.)
     */
-   if (IS_SRF_CALL((Node *) em->em_expr))
+   if (expression_returns_set((Node *) em->em_expr))
        return NULL;
 
    return em->em_expr;

diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 01cea102eab1a3b61a6b1ea91abf0cb5b26bb226..2c58bd0d6fb98443478b5acb98926e1fbfe58606 100644 (file)
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/tlist.c
@@ -21,6 +21,17 @@
 #include "optimizer/tlist.h"
 
 
+/*
+ * Test if an expression node represents a SRF call.  Beware multiple eval!
+ *
+ * Please note that this is only meant for use in split_pathtarget_at_srfs();
+ * if you use it anywhere else, your code is almost certainly wrong for SRFs
+ * nested within expressions.  Use expression_returns_set() instead.
+ */
+#define IS_SRF_CALL(node) \
+   ((IsA(node, FuncExpr) && ((FuncExpr *) (node))->funcretset) || \
+    (IsA(node, OpExpr) && ((OpExpr *) (node))->opretset))
+
 /*
  * Data structures for split_pathtarget_at_srfs().  To preserve the identity
  * of sortgroupref items even if they are textually equal(), what we track is

diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index 3c588def885b975a6831575a9391313d5237a844..4729cd01c89faab91b43489280535e4f984381ba 100644 (file)
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -24,11 +24,6 @@
 
 #include "nodes/parsenodes.h"
 
-/* Test if an expression node represents a SRF call.  Beware multiple eval! */
-#define IS_SRF_CALL(node) \
-   ((IsA(node, FuncExpr) && ((FuncExpr *) (node))->funcretset) || \
-    (IsA(node, OpExpr) && ((OpExpr *) (node))->opretset))
-
 /*
  * We don't want to include nodes/pathnodes.h here, because non-planner
  * code should generally treat PlannerInfo as an opaque typedef.