On 2017/08/02 15:21, Amit Langote wrote:
Thanks Fuita-san and Amit for reviewing.
On 2017/08/02 1:33, Amit Khandekar wrote:
On 1 August 2017 at 15:11, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote:
On 2017/07/31 18:56, Amit Langote wrote:
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.)
That's a good point, although it sounds like a bigger project that, IMHO,
should be undertaken separately, because that would involve designing for
considerations of expanding inheritance even in the INSERT case.
Agreed. I think that would be definitely a material for PG11.
I think the work of shifting to planner should be taken as a different
task when we shift the preparation of DispatchInfo to the planner.
Yeah, I think it'd be a good idea to do those projects together. That is,
doing what Fujita-san suggested and expanding partitioned tables in
partition bound order in the planner.
OK
-------
Few more comments :
@@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,
var->varlevelsup == context->sublevels_up)
{
/* Found a matching variable, make the substitution */
- Var *newvar = (Var *) palloc(sizeof(Var));
+ Var *newvar = copyObject(var);
int attno = var->varattno;
*newvar = *var;
Here, "*newvar = *var" should be removed.
Done.
I'm not sure this change is a good idea, because the copy by "*newvar =
*var" would be more efficient than the copyObject(). (We have this
optimization in other places as well. See eg, copyVar() in setrefs.c.)
Here are some other comments:
+ /* If the callers expects us to convert the
same, do so. */
+ if (OidIsValid(context->to_rowtype))
+ {
+ ConvertRowtypeExpr *r;
+
+ /* Var itself is converted to the
requested rowtype. */
+ newvar->vartype = context->to_rowtype;
+
+ /*
+ * And a conversion step on top to
convert back to the
+ * original type.
+ */
+ r = makeNode(ConvertRowtypeExpr);
+ r->arg = (Expr *) newvar;
+ r->resulttype = var->vartype;
+ r->convertformat = COERCE_IMPLICIT_CAST;
+ r->location = -1;
+
+ return (Node *) r;
+ }
Why not do this conversion if to_rowtype is valid and it's different
from the rowtype of the original whole-row Var like the previous patch?
Also, I think it's better to add an assertion that the rowtype of the
original whole-row Var is a named one. So, what I have in mind is:
if (OidIsValid(context->to_rowtype))
{
Assert(var->vartype != RECORDOID)
if (var->vartype != context->to_rowtype)
{
ConvertRowtypeExpr *r;
/* Var itself is converted to the requested rowtype. */
...
/* And a conversion step on top to convert back to the ... */
...
return (Node *) r;
}
}
Here is the modification to the map_variable_attnos()'s API:
map_variable_attnos(Node *node,
int target_varno, int sublevels_up,
const AttrNumber *attno_map,
int map_length,
- bool *found_whole_row)
+ bool *found_whole_row, Oid
to_rowtype)
This is nitpicking, but I think it would be better to put the new
argument to_rowtype right before the output argument found_whole_row.
+ * RelationGetRelType
+ * Returns the rel's pg_type OID.
+ */
+#define RelationGetRelType(relation) \
+ ((relation)->rd_rel->reltype)
This macro is used in only one place. Do we really need that? (This
macro would be useful for other places such as expand_inherited_rtentry,
but I think it's better to introduce this in a separate patch, maybe for
PG11.)
+-- check that wholerow vars in the RETUNING list work with partitioned
tables
Typo: s/RETUNING/RETURNING/
Sorry for the delay.
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
