proposal and patch : support INSERT INTO...RETURNING with partitioned table using rule
-----------------------------------
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
Attachments:
INSERT.returning_one.patchtext/x-diffDownload
--- 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
John Lumby <johnlumby@hotmail.com> wrote:
I attach patch based on clone of postgresql.git as of yesterday
Please read about the CommitFest process:
http://wiki.postgresql.org/wiki/CommitFest
and add your patch to the open CF:
https://commitfest.postgresql.org/action/commitfest_view/open
This will ensure that the patch doesn't get lost before the next review
cycle starts.
-Kevin
On Wed, Jun 20, 2012 at 12:24 PM, John Lumby <johnlumby@hotmail.com> wrote:
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.
This isn't rejected because the query is treated as condition; it's
rejected because it's not valid syntax. A SELECT query can't have a
RETURNING clause, because the target list (i.e. the part that
immediately follows the SELECT) already serves that purpose. The fact
that it's in a CREATE RULE statement is irrelevant:
rhaas=# select 4 returning 3;
ERROR: syntax error at or near "returning"
LINE 1: select 4 returning 3;
^
. 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.
That already (kind of) works:
rhaas=# create table history (id bigserial, name text);NOTICE: CREATE
TABLE will create implicit sequence "history_id_seq" for serial column
"history.id"
CREATE TABLE
rhaas=# create table history1 () inherits (history);
CREATE TABLE
rhaas=# create rule history_insert as on insert to history do instead
insert into history1 (id, name) values (NEW.id, NEW.name || ' is
awesome!') returning 17::bigint, 'cheeze whiz'::text;
CREATE RULE
rhaas=# insert into history (name) values ('Linus') returning id,
name; id | name
----+-------------
17 | cheeze whiz
(1 row)
INSERT 0 1
rhaas=# select * from history;
id | name
----+-------------------
1 | Linus is awesome!
(1 row)
I do notice that the RETURNING clause of the INSERT can't reference
NEW, which seems like a restriction that we probably ought to lift,
but it doesn't seem to have much to do with your patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company