----------------------------------- Problem I'm trying to solve: For partitioned tables, make it possible to use RETURNING clause on INSERT INTO together with DO INSTEAD rule
[ Note - wherever I say INSERT I also mean UPDATE and DELETE ] ----------------------------------- Current behaviour : An INSERT which has a RETURNING clause and which is to be rewritten based on a rule will be accepted if the rule is an "unconditional DO INSTEAD". In general I believe "unconditional" means "no WHERE clause", but in practice if the rule is of the form CREATE RULE insert_part_history as ON INSERT to history \ DO INSTEAD SELECT history_insert_partitioned(NEW) returning NEW.id this is treated as conditional and the query is rejected. Testcase: A table T is partitioned and has two or more columns, one of which is an id column declared as id bigint DEFAULT nextval('history_id_seq'::regclass) NOT NULL and the application issues "INSERT into history (column-list which excludes id) values (....) RETURNING id" I can get the re-direction of the INSERT *without* RETURNING to work using either trigger or rule, in which the trigger/rule invokes a procedure, but whichever way I do it, I could not get this RETURNING clause to work. For a trigger, the INSERT ... RETURNING was accepted but returned no rows, (as I would expect), and for the RULE, the INSERT ... RETURNING was rejected with : ERROR: cannot perform INSERT RETURNING on relation "history" HINT: You need an unconditional ON INSERT DO INSTEAD rule with a RETURNING clause. but this hint was not much help, since : For a rule, CREATE RULE insert_part_history as ON INSERT to history \ DO INSTEAD SELECT history_insert_partitioned(NEW) returning NEW.id ERROR: syntax error at or near "returning" LINE 1: ...DO INSTEAD SELECT history_insert_partitioned(NEW) returning ... Here the function history_insert_partitioned is something like CREATE FUNCTION history_insert_partitioned( NEW public.history) RETURNS BIGINT AS $$ DECLARE ... BEGIN ... < acccess NEW fields e.g. timestamp> < construct partitioned table name> < EXECUTE 'INSERT INTO ' partitioned table ... RETURN history_id; END; $$ LANGUAGE plpgsql ----------------------------------- Some references to discussion of this requirement : . http://wiki.postgresql.org/wiki/Todo item "Make it possible to use RETURNING together with conditional DO INSTEAD rules, such as for partitioning setups" . http://archives.postgresql.org/pgsql-general/2012-06/msg00377.php . http://archives.postgresql.org/pgsql-general/2010-12/msg00542.php . http://acodapella.blogspot.it/2011/06/hibernate-postgresql-table-partitioning.html ----------------------------------- Proposal: . I propose to extend the rule system to recognize cases where the INSERT query specifies RETURNING and the rule promises to return a row, and to then permit this query to run and return the expected row. In effect, to widen the definition of "unconditional" to handle cases such as my testcase. . One comment is that all the infrastructure for returning one row from the re-written query is already present in the code, and the non-trivial question is how to ensure the new design is safe in preventing any rewrite that actually would not return a row. . In this patch, I have chosen to make use of the LIMIT clause - I add a side-effect implication to a LIMIT clause when it occurs in a rewrite of an INSERT to mean "this rule will return a row". So, with my patch, same testcase, same function history_insert_partitioned and new rule CREATE RULE insert_part_history as ON INSERT to history \ DO INSTEAD SELECT history_insert_partitioned(NEW) LIMIT 1 the INSERT is accepted and returns the id. This use of LIMIT clause is probably contentious but I wished to avoid introducing new SQL syntax, and the LIMIT clause does have a connotation of returning rows. ----------------------------------- I attach patch based on clone of postgresql.git as of yesterday (120619-145751 EST) I have tested the patch with INSERT and UPDATE (not tested with DELETE but should work). The patch is not expected to be final but just to show how I did it. John
--- src/backend/optimizer/plan/planner.c.orig 2012-06-19 14:59:21.264574275 -0400 +++ src/backend/optimizer/plan/planner.c 2012-06-19 15:10:54.776590758 -0400 @@ -226,7 +226,8 @@ standard_planner(Query *parse, int curso result->commandType = parse->commandType; result->queryId = parse->queryId; - result->hasReturning = (parse->returningList != NIL); + result->returning_one = parse->returning_one; + result->hasReturning = ( (parse->returningList != NIL) || parse->returning_one ); result->hasModifyingCTE = parse->hasModifyingCTE; result->canSetTag = parse->canSetTag; result->transientPlan = glob->transientPlan; --- src/backend/rewrite/rewriteHandler.c.orig 2012-06-19 14:59:21.324574277 -0400 +++ src/backend/rewrite/rewriteHandler.c 2012-06-19 15:14:44.080596207 -0400 @@ -1734,6 +1734,8 @@ CopyAndAddInvertedQual(Query *parsetree, * (must be initialized to FALSE) * *qual_product - filled with modified original query if any qualified * INSTEAD rule is found (must be initialized to NULL) + * *limit_rows_ptr - hint that this rule will return rows : + * any value in range [ 1 , 23 ] indicates yes * Return value: * list of rule actions adjusted for use with this query * @@ -1753,11 +1755,16 @@ fireRules(Query *parsetree, List *locks, bool *instead_flag, bool *returning_flag, - Query **qual_product) + Query **qual_product + ,int64 *limit_rows_ptr + ) { List *results = NIL; ListCell *l; + if (limit_rows_ptr) + *limit_rows_ptr = -1; + foreach(l, locks) { RewriteRule *rule_lock = (RewriteRule *) lfirst(l); @@ -1810,6 +1817,22 @@ fireRules(Query *parsetree, { Query *rule_action = lfirst(r); + /* check for rule which is a SELECT with LIMIT clause - + ** if so, inform caller of the value (if const) or type of the limit + */ + if ( (limit_rows_ptr) + && (rule_action->commandType == CMD_SELECT) + && (rule_action->limitCount) + ) + { + if (nodeTag(rule_action->limitCount) == T_Const) { + *limit_rows_ptr = DatumGetInt64(((Const *)(rule_action->limitCount))->constvalue); + } + else if (nodeTag(rule_action->limitCount) == T_FuncExpr) { + *limit_rows_ptr = ((int64)((FuncExpr *)(rule_action->limitCount))->funcresulttype); + } + } + if (rule_action->commandType == CMD_NOTHING) continue; @@ -1850,6 +1873,7 @@ RewriteQuery(Query *parsetree, List *rew * clauses. (We have to do this first because the WITH clauses may get * copied into rule actions below.) */ + parsetree->returning_one = false; foreach(lc1, parsetree->cteList) { CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc1); @@ -1922,7 +1946,10 @@ RewriteQuery(Query *parsetree, List *rew RangeTblEntry *rt_entry; Relation rt_entry_relation; List *locks; + int64 limit_rows; /* has either the const value or the datatype of a limit value */ + /* which is a hint that this action will return specified number of rows */ + limit_rows = -1; result_relation = parsetree->resultRelation; Assert(result_relation != 0); rt_entry = rt_fetch(result_relation, parsetree->rtable); @@ -2002,7 +2029,9 @@ RewriteQuery(Query *parsetree, List *rew locks, &instead, &returning, - &qual_product); + &qual_product + ,&limit_rows + ); /* * If we got any product queries, recursively rewrite them --- but @@ -2044,14 +2073,28 @@ RewriteQuery(Query *parsetree, List *rew /* * If there is an INSTEAD, and the original query has a RETURNING, we - * have to have found a RETURNING in the rule(s), else fail. (Because - * DefineQueryRewrite only allows RETURNING in unconditional INSTEAD - * rules, there's no need to worry whether the substituted RETURNING - * will actually be executed --- it must be.) - */ - if ((instead || qual_product != NULL) && - parsetree->returningList && - !returning) + * have to have found in the rule(s) : + * EITHER a RETURNING + * OR a hint that this action will return specified number of rows + * else fail. + * (Because DefineQueryRewrite only allows RETURNING in unconditional + * INSTEAD rules, there's no need to worry whether the substituted + * RETURNING will actually be executed --- it must be.) + */ + if ( (instead || qual_product != NULL) + && (parsetree->returningList) + && (!returning) + ) + { + /* a value of limit_rows in the range 1 - 23 is a good hint + ** that query will return specified number of rows + */ + if ( (limit_rows >= 1) + && (limit_rows <= 23) + ) + { + parsetree->returning_one = true; + } else { switch (event) { @@ -2082,6 +2125,7 @@ RewriteQuery(Query *parsetree, List *rew break; } } + } heap_close(rt_entry_relation, NoLock); } @@ -2165,6 +2209,7 @@ QueryRewrite(Query *parsetree) bool foundOriginalQuery; Query *lastInstead; + parsetree->returning_one = false; /* * This function is only applied to top-level original queries */ @@ -2184,6 +2229,7 @@ QueryRewrite(Query *parsetree) * Apply all the RIR rules on each query * * This is also a handy place to mark each query with the original queryId + * and with returning_one */ results = NIL; foreach(l, querylist) @@ -2193,6 +2239,7 @@ QueryRewrite(Query *parsetree) query = fireRIRrules(query, NIL, false); query->queryId = input_query_id; + query->returning_one = parsetree->returning_one; results = lappend(results, query); } @@ -2233,9 +2280,13 @@ QueryRewrite(Query *parsetree) else { Assert(!query->canSetTag); - if (query->commandType == origCmdType && - (query->querySource == QSRC_INSTEAD_RULE || - query->querySource == QSRC_QUAL_INSTEAD_RULE)) + if ( ( (query->commandType == origCmdType) + || query->returning_one + ) + && ( (query->querySource == QSRC_INSTEAD_RULE) + || (query->querySource == QSRC_QUAL_INSTEAD_RULE) + ) + ) lastInstead = query; } } --- src/backend/nodes/copyfuncs.c.orig 2012-06-19 14:59:21.256574276 -0400 +++ src/backend/nodes/copyfuncs.c 2012-06-19 15:10:54.776590758 -0400 @@ -93,6 +93,7 @@ _copyPlannedStmt(const PlannedStmt *from COPY_NODE_FIELD(relationOids); COPY_NODE_FIELD(invalItems); COPY_SCALAR_FIELD(nParamExec); + COPY_SCALAR_FIELD(returning_one); return newnode; } @@ -2434,6 +2435,7 @@ _copyQuery(const Query *from) COPY_NODE_FIELD(rowMarks); COPY_NODE_FIELD(setOperations); COPY_NODE_FIELD(constraintDeps); + COPY_SCALAR_FIELD(returning_one); return newnode; } --- src/include/nodes/plannodes.h.orig 2012-06-19 14:59:21.496574281 -0400 +++ src/include/nodes/plannodes.h 2012-06-19 15:10:54.812590759 -0400 @@ -43,6 +43,8 @@ typedef struct PlannedStmt bool hasModifyingCTE; /* has insert|update|delete in WITH? */ + bool returning_one; /* inform portal that any intermediate result is to be returned - + when a RETURNING was rewritten */ bool canSetTag; /* do I set the command result tag? */ bool transientPlan; /* redo plan when TransactionXmin changes? */ --- src/include/nodes/parsenodes.h.orig 2012-06-19 14:59:21.492574281 -0400 +++ src/include/nodes/parsenodes.h 2012-06-19 15:10:54.816590759 -0400 @@ -105,6 +105,8 @@ typedef struct Query uint32 queryId; /* query identifier (can be set by plugins) */ + bool returning_one; /* inform portal that any intermediate result is to be returned - + when a RETURNING was rewritten */ bool canSetTag; /* do I set the command result tag? */ Node *utilityStmt; /* non-null if this is DECLARE CURSOR or a
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers