Re: subquery results bypassed

Anthony Wood (woody+postgresqlbugs(at)switchonline(dot)com(dot)au) writes:
> [ SELECT DISTINCT ON in a subquery-in-FROM misbehaves ]

Here's the patch against 7.1.2 to fix this problem. This also fixes a
related problem noted a few days ago, that outer WHERE clauses shouldn't
be pushed down into a sub-select that has a LIMIT clause.

regards, tom lane

*** src/backend/optimizer/path/allpaths.c.orig Wed Mar 21 22:59:34 2001
--- src/backend/optimizer/path/allpaths.c Tue Jul 31 14:05:05 2001
***************
*** 125,135 ****
* Non-pushed-down clauses will get evaluated as qpquals of
* the SubqueryScan node.
*
* XXX Are there any cases where we want to make a policy
* decision not to push down, because it'd result in a worse
* plan?
*/
! if (rte->subquery->setOperations == NULL)
{
/* OK to consider pushing down individual quals */
List *upperrestrictlist = NIL;
--- 125,141 ----
* Non-pushed-down clauses will get evaluated as qpquals of
* the SubqueryScan node.
*
+ * We can't push down into subqueries with LIMIT or DISTINCT ON
+ * clauses, either.
+ *
* XXX Are there any cases where we want to make a policy
* decision not to push down, because it'd result in a worse
* plan?
*/
! if (rte->subquery->setOperations == NULL &&
! rte->subquery->limitOffset == NULL &&
! rte->subquery->limitCount == NULL &&
! !has_distinct_on_clause(rte->subquery))
{
/* OK to consider pushing down individual quals */
List *upperrestrictlist = NIL;
*** src/backend/optimizer/util/clauses.c.orig Tue Mar 27 12:12:34 2001
--- src/backend/optimizer/util/clauses.c Tue Jul 31 14:05:01 2001
***************
*** 730,742 ****


/*****************************************************************************
* *
* General clause-manipulating routines *
* *
*****************************************************************************/

/*
! * clause_relids_vars
* Retrieves distinct relids and vars appearing within a clause.
*
* '*relids' is set to an integer list of all distinct "varno"s appearing
--- 730,794 ----


/*****************************************************************************
+ * Tests on clauses of queries
+ *
+ * Possibly this code should go someplace else, since this isn't quite the
+ * same meaning of "clause" as is used elsewhere in this module. But I can't
+ * think of a better place for it...
+ *****************************************************************************/
+
+ /*
+ * Test whether a query uses DISTINCT ON, ie, has a distinct-list that is
+ * just a subset of the output columns.
+ */
+ bool
+ has_distinct_on_clause(Query *query)
+ {
+ List *targetList;
+
+ /* Is there a DISTINCT clause at all? */
+ if (query->distinctClause == NIL)
+ return false;
+ /*
+ * If the DISTINCT list contains all the nonjunk targetlist items,
+ * then it's a simple DISTINCT, else it's DISTINCT ON. We do not
+ * require the lists to be in the same order (since the parser may
+ * have adjusted the DISTINCT clause ordering to agree with ORDER BY).
+ */
+ foreach(targetList, query->targetList)
+ {
+ TargetEntry *tle = (TargetEntry *) lfirst(targetList);
+ Index ressortgroupref;
+ List *distinctClause;
+
+ if (tle->resdom->resjunk)
+ continue;
+ ressortgroupref = tle->resdom->ressortgroupref;
+ if (ressortgroupref == 0)
+ return true; /* definitely not in DISTINCT list */
+ foreach(distinctClause, query->distinctClause)
+ {
+ SortClause *scl = (SortClause *) lfirst(distinctClause);
+
+ if (scl->tleSortGroupRef == ressortgroupref)
+ break; /* found TLE in DISTINCT */
+ }
+ if (distinctClause == NIL)
+ return true; /* this TLE is not in DISTINCT list */
+ }
+ /* It's a simple DISTINCT */
+ return false;
+ }
+
+
+ /*****************************************************************************
* *
* General clause-manipulating routines *
* *
*****************************************************************************/

/*
! * clause_get_relids_vars
* Retrieves distinct relids and vars appearing within a clause.
*
* '*relids' is set to an integer list of all distinct "varno"s appearing
*** src/backend/utils/adt/ruleutils.c.orig Wed Apr 18 13:04:24 2001
--- src/backend/utils/adt/ruleutils.c Tue Jul 31 14:04:51 2001
***************
*** 119,125 ****
static void get_basic_select_query(Query *query, deparse_context *context);
static void get_setop_query(Node *setOp, Query *query,
deparse_context *context, bool toplevel);
- static bool simple_distinct(List *distinctClause, List *targetList);
static void get_rule_sortgroupclause(SortClause *srt, List *tlist,
bool force_colno,
deparse_context *context);
--- 119,124 ----
***************
*** 1006,1014 ****
/* Add the DISTINCT clause if given */
if (query->distinctClause != NIL)
{
! if (simple_distinct(query->distinctClause, query->targetList))
! appendStringInfo(buf, " DISTINCT");
! else
{
appendStringInfo(buf, " DISTINCT ON (");
sep = "";
--- 1005,1011 ----
/* Add the DISTINCT clause if given */
if (query->distinctClause != NIL)
{
! if (has_distinct_on_clause(query))
{
appendStringInfo(buf, " DISTINCT ON (");
sep = "";
***************
*** 1023,1028 ****
--- 1020,1027 ----
}
appendStringInfo(buf, ")");
}
+ else
+ appendStringInfo(buf, " DISTINCT");
}

/* Then we tell what to select (the targetlist) */
***************
*** 1147,1180 ****
elog(ERROR, "get_setop_query: unexpected node %d",
(int) nodeTag(setOp));
}
- }
-
- /*
- * Detect whether a DISTINCT list can be represented as just DISTINCT
- * or needs DISTINCT ON. It's simple if it contains exactly the nonjunk
- * targetlist items.
- */
- static bool
- simple_distinct(List *distinctClause, List *targetList)
- {
- while (targetList)
- {
- TargetEntry *tle = (TargetEntry *) lfirst(targetList);
-
- if (!tle->resdom->resjunk)
- {
- if (distinctClause == NIL)
- return false;
- if (((SortClause *) lfirst(distinctClause))->tleSortGroupRef !=
- tle->resdom->ressortgroupref)
- return false;
- distinctClause = lnext(distinctClause);
- }
- targetList = lnext(targetList);
- }
- if (distinctClause != NIL)
- return false;
- return true;
}

/*
--- 1146,1151 ----
*** src/include/optimizer/clauses.h.orig Wed Mar 21 23:00:53 2001
--- src/include/optimizer/clauses.h Tue Jul 31 14:04:35 2001
***************
*** 59,64 ****
--- 59,66 ----
extern bool is_pseudo_constant_clause(Node *clause);
extern List *pull_constant_clauses(List *quals, List **constantQual);

+ extern bool has_distinct_on_clause(Query *query);
+
extern void clause_get_relids_vars(Node *clause, Relids *relids, List **vars);
extern int NumRelids(Node *clause);
extern void get_relattval(Node *clause, int targetrelid,