diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 8810ae005a2e464ee413e9d7078d3acdea1fb20f..8f8e65c374a52a22fd268348fe9c9124ad8a174a 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v 1.74 1999/02/22 19:55:42 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v 1.75 1999/03/01 00:10:30 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -326,16 +326,6 @@ _copyMergeJoin(MergeJoin *from) */ Node_Copy(from, newnode, mergeclauses); - newnode->mergejoinop = from->mergejoinop; - - newnode->mergerightorder = (Oid *) palloc(sizeof(Oid) * 2); - newnode->mergerightorder[0] = from->mergerightorder[0]; - newnode->mergerightorder[1] = 0; - - newnode->mergeleftorder = (Oid *) palloc(sizeof(Oid) * 2); - newnode->mergeleftorder[0] = from->mergeleftorder[0]; - newnode->mergeleftorder[1] = 0; - return newnode; } diff --git a/src/backend/nodes/freefuncs.c b/src/backend/nodes/freefuncs.c index dd3f040ad4534ad19eb1ae0ec70e5d00dde1c8a4..c41c5fb4318d7bacba338d1b6ee9853d87a1e831 100644 --- a/src/backend/nodes/freefuncs.c +++ b/src/backend/nodes/freefuncs.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/nodes/Attic/freefuncs.c,v 1.14 1999/02/22 19:55:42 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/nodes/Attic/freefuncs.c,v 1.15 1999/03/01 00:10:31 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -267,9 +267,6 @@ _freeMergeJoin(MergeJoin *node) */ freeObject(node->mergeclauses); - pfree(node->mergerightorder); - pfree(node->mergeleftorder); - pfree(node); } diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 78c90ff420720746ecaaa6a5108702abfd4406b6..11f79e4722001f0870225a38545c904576787a5b 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -5,7 +5,7 @@ * * Copyright (c) 1994, Regents of the University of California * - * $Id: outfuncs.c,v 1.76 1999/02/23 08:01:47 thomas Exp $ + * $Id: outfuncs.c,v 1.77 1999/03/01 00:10:31 tgl Exp $ * * NOTES * Every (plan) node in POSTGRES has an associated "out" routine which @@ -371,12 +371,6 @@ _outMergeJoin(StringInfo str, MergeJoin *node) appendStringInfo(str, " :mergeclauses "); _outNode(str, node->mergeclauses); - - appendStringInfo(str, - " :mergejoinop %u :mergerightorder %u :mergeleftorder %u ", - node->mergejoinop, - node->mergerightorder, - node->mergeleftorder); } /* diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index f0fa81a4e2e417443005803680e5aa36b714a374..3e1dca5a345860cad9c727efb4367fd477d9ed24 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/nodes/readfuncs.c,v 1.59 1999/02/18 00:49:15 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/nodes/readfuncs.c,v 1.60 1999/03/01 00:10:31 tgl Exp $ * * NOTES * Most of the read functions for plan nodes are tested. (In fact, they @@ -426,10 +426,6 @@ _readMergeJoin() token = lsptok(NULL, &length); /* eat :mergeclauses */ local_node->mergeclauses = nodeRead(true); /* now read it */ - token = lsptok(NULL, &length); /* eat :mergejoinop */ - token = lsptok(NULL, &length); /* get mergejoinop */ - local_node->mergejoinop = atol(token); - return local_node; } diff --git a/src/backend/optimizer/path/mergeutils.c b/src/backend/optimizer/path/mergeutils.c index 7d0009c3e8b1f39d1a98665a1d6405a0fe07e5a0..5f5a11bc7874406a13deca1c206cf979ed00bfed 100644 --- a/src/backend/optimizer/path/mergeutils.c +++ b/src/backend/optimizer/path/mergeutils.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/path/Attic/mergeutils.c,v 1.19 1999/02/15 03:22:06 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/path/Attic/mergeutils.c,v 1.20 1999/03/01 00:10:32 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -27,6 +27,14 @@ * it within a mergeinfo node containing other clause nodes with the same * mergejoin ordering. * + * XXX This is completely braindead: there is no reason anymore to segregate + * mergejoin clauses by join operator, since the executor can handle mergejoin + * clause sets with different operators in them. Instead, we ought to be + * building a MergeInfo for each potentially useful ordering of the input + * relations. But right now the optimizer's internal data structures do not + * support that (MergeInfo can only store one MergeOrder for a set of clauses). + * Something to fix next time... + * * 'restrictinfo_list' is the list of restrictinfo nodes * 'inner_relid' is the relid of the inner join relation * @@ -38,7 +46,7 @@ group_clauses_by_order(List *restrictinfo_list, int inner_relid) { List *mergeinfo_list = NIL; - List *xrestrictinfo = NIL; + List *xrestrictinfo; foreach(xrestrictinfo, restrictinfo_list) { @@ -84,10 +92,10 @@ group_clauses_by_order(List *restrictinfo_list, mergeinfo_list); } - ((JoinMethod *) xmergeinfo)->clauses = lcons(clause, - ((JoinMethod *) xmergeinfo)->clauses); - ((JoinMethod *) xmergeinfo)->jmkeys = lcons(jmkeys, - ((JoinMethod *) xmergeinfo)->jmkeys); + xmergeinfo->jmethod.clauses = lcons(clause, + xmergeinfo->jmethod.clauses); + xmergeinfo->jmethod.jmkeys = lcons(jmkeys, + xmergeinfo->jmethod.jmkeys); } } return mergeinfo_list; diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 68e40a4e08f9e9251716a03544ec1087ed0cd840..10d7f559130673d264b7da0cf108841847952add 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/plan/createplan.c,v 1.49 1999/02/21 03:48:45 scrappy Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/plan/createplan.c,v 1.50 1999/03/01 00:10:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -44,6 +44,8 @@ #define NONAME_MATERIAL 2 static List *switch_outer(List *clauses); +static Oid *generate_merge_input_sortorder(List *pathkeys, + MergeOrder *mergeorder); static Scan *create_scan_node(Path *best_path, List *tlist); static Join *create_join_node(JoinPath *best_path, List *tlist); static SeqScan *create_seqscan_node(Path *best_path, List *tlist, @@ -70,8 +72,7 @@ static HashJoin *make_hashjoin(List *tlist, List *qpqual, List *hashclauses, Plan *lefttree, Plan *righttree); static Hash *make_hash(List *tlist, Var *hashkey, Plan *lefttree); static MergeJoin *make_mergejoin(List *tlist, List *qpqual, - List *mergeclauses, Oid opcode, Oid *rightorder, - Oid *leftorder, Plan *righttree, Plan *lefttree); + List *mergeclauses, Plan *righttree, Plan *lefttree); static Material *make_material(List *tlist, Oid nonameid, Plan *lefttree, int keycount); @@ -505,9 +506,6 @@ create_mergejoin_node(MergePath *best_path, { List *qpqual, *mergeclauses; - RegProcedure opcode; - Oid *outer_order, - *inner_order; MergeJoin *join_node; @@ -528,27 +526,20 @@ create_mergejoin_node(MergePath *best_path, outer_tlist, inner_tlist)); - opcode = get_opcode((best_path->jpath.path.pathorder->ord.merge)->join_operator); - - outer_order = (Oid *) palloc(sizeof(Oid) * 2); - outer_order[0] = (best_path->jpath.path.pathorder->ord.merge)->left_operator; - outer_order[1] = 0; - - inner_order = (Oid *) palloc(sizeof(Oid) * 2); - inner_order[0] = (best_path->jpath.path.pathorder->ord.merge)->right_operator; - inner_order[1] = 0; - /* * Create explicit sort paths for the outer and inner join paths if * necessary. The sort cost was already accounted for in the path. */ if (best_path->outersortkeys) { + Oid *outer_order = generate_merge_input_sortorder( + best_path->outersortkeys, + best_path->jpath.path.pathorder->ord.merge); Noname *sorted_outer_node = make_noname(outer_tlist, - best_path->outersortkeys, - outer_order, - outer_node, - NONAME_SORT); + best_path->outersortkeys, + outer_order, + outer_node, + NONAME_SORT); sorted_outer_node->plan.cost = outer_node->cost; outer_node = (Plan *) sorted_outer_node; @@ -556,22 +547,22 @@ create_mergejoin_node(MergePath *best_path, if (best_path->innersortkeys) { + Oid *inner_order = generate_merge_input_sortorder( + best_path->innersortkeys, + best_path->jpath.path.pathorder->ord.merge); Noname *sorted_inner_node = make_noname(inner_tlist, best_path->innersortkeys, inner_order, inner_node, NONAME_SORT); - sorted_inner_node->plan.cost = outer_node->cost; + sorted_inner_node->plan.cost = outer_node->cost; /* XXX not inner_node? */ inner_node = (Plan *) sorted_inner_node; } join_node = make_mergejoin(tlist, qpqual, mergeclauses, - opcode, - inner_order, - outer_order, inner_node, outer_node); @@ -662,7 +653,7 @@ fix_indxqual_references(Node *clause, Path *index_path) pos++; } } - newclause = copyObject((Node *) clause); + newclause = copyObject(clause); ((Var *) newclause)->varattno = pos + 1; return newclause; } @@ -760,24 +751,22 @@ fix_indxqual_references(Node *clause, Path *index_path) * switch_outer * Given a list of merge clauses, rearranges the elements within the * clauses so the outer join variable is on the left and the inner is on - * the right. - * - * Returns the rearranged list ? - * - * XXX Shouldn't the operator be commuted?! + * the right. The original list is not touched; a modified list + * is returned. */ static List * switch_outer(List *clauses) { List *t_list = NIL; - Expr *temp = NULL; - List *i = NIL; + Expr *temp; + List *i; Expr *clause; Node *op; foreach(i, clauses) { clause = lfirst(i); + Assert(is_opclause((Node*) clause)); op = (Node *) get_rightop(clause); Assert(op != (Node*) NULL); if (IsA(op, ArrayRef)) @@ -785,10 +774,16 @@ switch_outer(List *clauses) Assert(IsA(op, Var)); if (var_is_outer((Var *) op)) { + /* Duplicate just enough of the structure to allow commuting + * the clause without changing the original list. Could use + * copyObject, but a complete deep copy is overkill. + */ temp = make_clause(clause->opType, clause->oper, - lcons(get_rightop(clause), - lcons(get_leftop(clause), + lcons(get_leftop(clause), + lcons(get_rightop(clause), NIL))); + /* Commute it --- note this modifies the temp node in-place. */ + CommuteClause((Node *) temp); t_list = lappend(t_list, temp); } else @@ -797,6 +792,45 @@ switch_outer(List *clauses) return t_list; } +/* + * generate_merge_input_sortorder + * + * Generate the list of sort ops needed to sort one of the input paths for + * a merge. We may have to use either left or right sortop for each item, + * since the original mergejoin clause may or may not have been commuted + * (compare switch_outer above). + * + * XXX This is largely a crock. It works only because group_clauses_by_order + * only groups together mergejoin clauses that have identical MergeOrder info, + * which means we can safely use a single MergeOrder input to deal with all + * the data. There should be a more general data structure that allows coping + * with groups of mergejoin clauses that have different join operators. + */ +static Oid * +generate_merge_input_sortorder(List *pathkeys, MergeOrder *mergeorder) +{ + int listlength = length(pathkeys); + Oid *result = (Oid*) palloc(sizeof(Oid) * (listlength+1)); + Oid *nextsortop = result; + List *p; + + foreach(p, pathkeys) + { + Var *pkey = (Var*) lfirst((List*) lfirst(p)); + Assert(IsA(pkey, Var)); + if (pkey->vartype == mergeorder->left_type) + *nextsortop++ = mergeorder->left_operator; + else if (pkey->vartype == mergeorder->right_type) + *nextsortop++ = mergeorder->right_operator; + else + elog(ERROR, + "generate_merge_input_sortorder: can't handle data type %d", + pkey->vartype); + } + *nextsortop++ = InvalidOid; + return result; +} + /* * set_noname_tlist_operators * Sets the key and keyop fields of resdom nodes in a target list. @@ -806,18 +840,16 @@ switch_outer(List *clauses) * corresponding to vars in the target list that are to * be sorted or hashed * 'operators' is the corresponding list of N sort or hash operators - * 'keyno' is the first key number - * XXX - keyno ? doesn't exist - jeff * - * Returns the modified target list. + * Returns the modified-in-place target list. */ static List * set_noname_tlist_operators(List *tlist, List *pathkeys, Oid *operators) { - Node *pathkey = NULL; int keyno = 1; - Resdom *resdom = (Resdom *) NULL; - List *i = NIL; + Node *pathkey; + Resdom *resdom; + List *i; foreach(i, pathkeys) { @@ -828,12 +860,9 @@ set_noname_tlist_operators(List *tlist, List *pathkeys, Oid *operators) /* * Order the resdom pathkey and replace the operator OID for each * key with the regproc OID. - * - * XXX Note that the optimizer only generates merge joins with 1 - * operator (see create_mergejoin_node) - ay 2/95 */ resdom->reskey = keyno; - resdom->reskeyop = get_opcode(operators[0]); + resdom->reskeyop = get_opcode(operators[keyno-1]); } keyno += 1; } @@ -1024,9 +1053,6 @@ static MergeJoin * make_mergejoin(List *tlist, List *qpqual, List *mergeclauses, - Oid opcode, - Oid *rightorder, - Oid *leftorder, Plan *righttree, Plan *lefttree) { @@ -1041,9 +1067,6 @@ make_mergejoin(List *tlist, plan->lefttree = lefttree; plan->righttree = righttree; node->mergeclauses = mergeclauses; - node->mergejoinop = opcode; - node->mergerightorder = rightorder; - node->mergeleftorder = leftorder; return node; } diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 3fb0a901a045c0137aae182e9f2f194edbd09403..113f78103b629a974a965324336475f02e29eae2 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.31 1999/02/18 00:49:37 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.32 1999/03/01 00:10:35 tgl Exp $ * * HISTORY * AUTHOR DATE MAJOR EVENT @@ -780,24 +780,25 @@ CommuteClause(Node *clause) HeapTuple heapTup; if (!is_opclause(clause)) - return; + elog(ERROR, "CommuteClause: applied to non-operator clause"); heapTup = (HeapTuple) get_operator_tuple(get_commutator(((Oper *) ((Expr *) clause)->oper)->opno)); if (heapTup == (HeapTuple) NULL) - return; + elog(ERROR, "CommuteClause: no commutator for operator %d", + ((Oper *) ((Expr *) clause)->oper)->opno); commuTup = (Form_pg_operator) GETSTRUCT(heapTup); commu = makeOper(heapTup->t_data->t_oid, - InvalidOid, + commuTup->oprcode, commuTup->oprresult, ((Oper *) ((Expr *) clause)->oper)->opsize, NULL); /* - * reform the clause -> (operator func/var constant) + * re-form the clause in-place! */ ((Expr *) clause)->oper = (Node *) commu; temp = lfirst(((Expr *) clause)->args); diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 40883fa7a866345cdd02dc79dc8332ec02c3f678..3d3b2ca917d5cdc8942d23b9d4c6e960d4c58e06 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -6,7 +6,7 @@ * * Copyright (c) 1994, Regents of the University of California * - * $Id: plannodes.h,v 1.22 1999/02/13 23:21:40 momjian Exp $ + * $Id: plannodes.h,v 1.23 1999/03/01 00:10:36 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -208,9 +208,6 @@ typedef struct MergeJoin { Join join; List *mergeclauses; - Oid mergejoinop; - Oid *mergerightorder;/* inner sort operator */ - Oid *mergeleftorder; /* outer sort operator */ MergeJoinState *mergestate; } MergeJoin; diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out index 92cb1be9485412867ffd3371f9bbf7595ece508e..b720c5d9cbbccf66658789824a0e1e48ec16162d 100644 --- a/src/test/regress/expected/opr_sanity.out +++ b/src/test/regress/expected/opr_sanity.out @@ -51,7 +51,7 @@ WHERE p1.oid != p2.oid AND p1.oprkind = p2.oprkind AND p1.oprleft = p2.oprleft AND p1.oprright = p2.oprright; -oid|oprcode|oid|oprcode +oid|oprcode|oid|oprcode ---+-------+---+------- (0 rows) @@ -108,3 +108,32 @@ oid|oprcode|oid|oprcode ---+-------+---+------- (0 rows) +QUERY: SELECT p1.oid, p1.* FROM pg_operator AS p1 +WHERE p1.oprlsortop != 0 AND + p1.oprcom = 0; +oid|oprname|oprowner|oprprec|oprkind|oprisleft|oprcanhash|oprleft|oprright|oprresult|oprcom|oprnegate|oprlsortop|oprrsortop|oprcode|oprrest|oprjoin +---+-------+--------+-------+-------+---------+----------+-------+--------+---------+------+---------+----------+----------+-------+-------+------- +(0 rows) + +QUERY: SELECT p1.oid, p1.* FROM pg_operator AS p1 +WHERE p1.oprlsortop != 0 AND NOT + EXISTS(SELECT * FROM pg_operator AS p2 WHERE + p2.oprname = '<' AND + p2.oprleft = p1.oprleft AND + p2.oprright = p1.oprright AND + p2.oprkind = 'b'); +oid|oprname|oprowner|oprprec|oprkind|oprisleft|oprcanhash|oprleft|oprright|oprresult|oprcom|oprnegate|oprlsortop|oprrsortop|oprcode|oprrest|oprjoin +---+-------+--------+-------+-------+---------+----------+-------+--------+---------+------+---------+----------+----------+-------+-------+------- +(0 rows) + +QUERY: SELECT p1.oid, p1.* FROM pg_operator AS p1 +WHERE p1.oprlsortop != 0 AND NOT + EXISTS(SELECT * FROM pg_operator AS p2 WHERE + p2.oprname = '>' AND + p2.oprleft = p1.oprleft AND + p2.oprright = p1.oprright AND + p2.oprkind = 'b'); +oid|oprname|oprowner|oprprec|oprkind|oprisleft|oprcanhash|oprleft|oprright|oprresult|oprcom|oprnegate|oprlsortop|oprrsortop|oprcode|oprrest|oprjoin +---+-------+--------+-------+-------+---------+----------+-------+--------+---------+------+---------+----------+----------+-------+-------+------- +(0 rows) + diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql index c2567d294a98fb6864e81dfad30e262ce90e5b18..1a522468163d0f8b3deae402237c35be8274d43c 100644 --- a/src/test/regress/sql/opr_sanity.sql +++ b/src/test/regress/sql/opr_sanity.sql @@ -81,8 +81,8 @@ WHERE p1.oprnegate = p2.oid AND p2.oprresult != 16 OR p1.oid != p2.oprnegate); --- Look for mergesort operators that don't match. --- A mergesort link leads from an '=' operator to the +-- Look for mergejoin operators that don't match their links. +-- A mergejoin link leads from an '=' operator to the -- sort operator ('<' operator) that's appropriate for -- its left-side or right-side data type. @@ -107,3 +107,29 @@ WHERE p1.oprrsortop = p2.oid AND p1.oprresult != 16 OR p2.oprresult != 16 OR p1.oprlsortop = 0); + +-- A mergejoinable = operator must have a commutator (usually itself) +-- as well as corresponding < and > operators. Note that the "corresponding" +-- operators have the same L and R input datatypes as the = operator, +-- whereas the operators linked to by oprlsortop and oprrsortop have input +-- datatypes L,L and R,R respectively. + +SELECT p1.oid, p1.* FROM pg_operator AS p1 +WHERE p1.oprlsortop != 0 AND + p1.oprcom = 0; + +SELECT p1.oid, p1.* FROM pg_operator AS p1 +WHERE p1.oprlsortop != 0 AND NOT + EXISTS(SELECT * FROM pg_operator AS p2 WHERE + p2.oprname = '<' AND + p2.oprleft = p1.oprleft AND + p2.oprright = p1.oprright AND + p2.oprkind = 'b'); + +SELECT p1.oid, p1.* FROM pg_operator AS p1 +WHERE p1.oprlsortop != 0 AND NOT + EXISTS(SELECT * FROM pg_operator AS p2 WHERE + p2.oprname = '>' AND + p2.oprleft = p1.oprleft AND + p2.oprright = p1.oprright AND + p2.oprkind = 'b');