On 2017/07/31 18:56, Amit Langote wrote:
On 2017/07/28 20:46, Amit Khandekar wrote:
create table foo (a int, b text) partition by list (a);
create table foo1 partition of foo for values in (1);
create table foo2(b text, a int) ;
alter table foo attach partition foo2 for values in (2);
postgres=# insert into foo values (1, 'hi there');
INSERT 0 1
postgres=# insert into foo values (2, 'bi there');
INSERT 0 1
postgres=# insert into foo values (2, 'error there') returning foo;
ERROR: table row type and query-specified row type do not match
DETAIL: Table has type text at ordinal position 1, but query expects integer.
This is due to a mismatch between the composite type tuple descriptor
of the leaf partition doesn't match the RETURNING slot tuple
descriptor.
We probably need to do what
inheritance_planner()=>adjust_appendrel_attrs() does for adjusting the
attrs in the child rel parse tree; i.e., handle some specific nodes
other than just simple var nodes. In adjust_appendrel_attrs_mutator(),
for whole row node, it updates var->vartype to the child rel.
Yes, that's what's needed here. So we need to teach
map_variable_attnos_mutator() to convert whole-row vars just like it's
done in adjust_appendrel_attrs_mutator().
Seems reasonable. (Though I think it might be better to do this kind of
conversion in the planner, not the executor, because that would increase
the efficiency of cached plans.)
I suspect that the other nodes that adjust_appendrel_attrs_mutator
handles (like JoinExpr, CurrentOfExpr, etc ) may also require similar
adjustments for our case, because WithCheckOptions (and I think even
RETURNING) can have a subquery.
Actually, WITH CHECK and RETURNING expressions that are processed in the
executor (i.e., in ExecInitModifyTable) are finished plan tree expressions
(not parse or rewritten parse tree expressions), so we need not have to
worry about having to convert those things in this case.
Remember that adjust_appendrel_attrs_mutator() has to handle raw Query
trees, because we plan the whole query separately for each partition in
the UPDATE and DELETE (inheritance_planner). Since we don't need to plan
an INSERT query for each partition separately (at least without the
foreign tuple-routing support), we need not worry about converting
anything beside Vars (with proper support for converting whole-row ones
which you discovered has been missing), which we can do within the
executor on the finished plan tree expressions.
Yeah, sublinks in WITH CHECK OPTION and RETURNING would already have
been converted to subplans by the planner, so we don't need to worry
about recursing into subqueries in sublinks at the execution time.
Attached 2 patches:
0001: Addresses the bug that Rajkumar reported (handling whole-row vars in
WITH CHECK and RETURNING expressions at all)
0002: Addressed the bug that Amit reported (converting whole-row vars
that could occur in WITH CHECK and RETURNING expressions)
I took a quick look at the patches. One thing I noticed is this:
map_variable_attnos(Node *node,
int target_varno, int sublevels_up,
const AttrNumber *attno_map, int
map_length,
+ Oid from_rowtype, Oid to_rowtype,
bool *found_whole_row)
{
map_variable_attnos_context context;
@@ -1291,6 +1318,8 @@ map_variable_attnos(Node *node,
context.sublevels_up = sublevels_up;
context.attno_map = attno_map;
context.map_length = map_length;
+ context.from_rowtype = from_rowtype;
+ context.to_rowtype = to_rowtype;
context.found_whole_row = found_whole_row;
You added two arguments to pass to map_variable_attnos(): from_rowtype
and to_rowtype. But I'm not sure that we really need the from_rowtype
argument because it's possible to get the Oid from the vartype of target
whole-row Vars. And in this:
+ /*
+ * If the callers expects us to convert the
same, do so if
+ * necessary.
+ */
+ if (OidIsValid(context->to_rowtype) &&
+ OidIsValid(context->from_rowtype) &&
+ context->to_rowtype !=
context->from_rowtype)
+ {
+ ConvertRowtypeExpr *r =
makeNode(ConvertRowtypeExpr);
+
+ r->arg = (Expr *) newvar;
+ r->resulttype = context->from_rowtype;
+ r->convertformat = COERCE_IMPLICIT_CAST;
+ r->location = -1;
+ /* Make sure the Var node has the right
type ID, too */
+ newvar->vartype = context->to_rowtype;
+ return (Node *) r;
+ }
I think we could set r->resulttype to the vartype (ie, "r->resulttype =
newvar->vartype" instead of "r->resulttype = context->from_rowtype").
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
