(2018/03/18 13:17), Alvaro Herrera wrote:
Alvaro Herrera wrote: The only thing that I remain unhappy about this patch is the whole adjust_and_expand_partition_tlist() thing. I fear we may be doing redundant and/or misplaced work. I'll look into it next week.
I'm still reviewing the patches, but I really agree on that point. As Pavan mentioned upthread, the onConflictSet tlist for the root parent, from which we create a translated onConflictSet tlist for a partition, would have already been processed by expand_targetlist() to contain all missing columns as well, so I think we could create the tlist for the partition by simply re-ordering the expression-converted tlist (ie, conv_setproj) based on the conversion map for the partition. The Attached defines a function for that, which could be called, instead of calling adjust_and_expand_partition_tlist(). This would allow us to get rid of planner changes from the patches. Maybe I'm missing something, though.
Best regards, Etsuro Fujita
*** src/backend/executor/execPartition.c.org 2018-03-19 21:32:52.000000000 +0900 --- src/backend/executor/execPartition.c 2018-03-19 21:44:17.000000000 +0900 *************** *** 15,24 **** --- 15,26 ---- #include "postgres.h" #include "catalog/pg_inherits_fn.h" + #include "catalog/pg_type.h" #include "executor/execPartition.h" #include "executor/executor.h" #include "mb/pg_wchar.h" #include "miscadmin.h" + #include "nodes/makefuncs.h" #include "optimizer/prep.h" #include "utils/lsyscache.h" #include "utils/rls.h" *************** *** 37,42 **** --- 39,45 ---- Datum *values, bool *isnull, int maxfieldlen); + static List *adjust_onconflictset_tlist(List *tlist, TupleConversionMap *map); /* * ExecSetupPartitionTupleRouting - sets up information needed during *************** *** 554,565 **** firstVarno, partrel, firstResultRel, NULL); ! conv_setproj = ! adjust_and_expand_partition_tlist(RelationGetDescr(firstResultRel), ! RelationGetDescr(partrel), ! RelationGetRelationName(partrel), ! firstVarno, ! conv_setproj); tupDesc = ExecTypeFromTL(conv_setproj, partrelDesc->tdhasoid); part_conflproj_slot = ExecInitExtraTupleSlot(mtstate->ps.state, --- 557,563 ---- firstVarno, partrel, firstResultRel, NULL); ! conv_setproj = adjust_onconflictset_tlist(conv_setproj, map); tupDesc = ExecTypeFromTL(conv_setproj, partrelDesc->tdhasoid); part_conflproj_slot = ExecInitExtraTupleSlot(mtstate->ps.state, *************** *** 1091,1093 **** --- 1089,1153 ---- return buf.data; } + + /* + * Adjust the targetlist entries of a translated ON CONFLICT UPDATE operation + * + * The expressions have already been fixed, but we need to re-order the tlist + * so that the target resnos match the child table. + * + * The given tlist has already been through expression_tree_mutator; + * therefore the TargetEntry nodes are fresh copies that it's okay to + * scribble on. + */ + static List * + adjust_onconflictset_tlist(List *tlist, TupleConversionMap *map) + { + List *new_tlist = NIL; + TupleDesc tupdesc = map->outdesc; + AttrNumber *attrMap = map->attrMap; + int numattrs = tupdesc->natts; + int attrno; + + for (attrno = 1; attrno <= numattrs; attrno++) + { + Form_pg_attribute att_tup = TupleDescAttr(tupdesc, attrno - 1); + TargetEntry *tle; + + if (attrMap[attrno - 1] != 0) + { + Assert(!att_tup->attisdropped); + + /* Get the corresponding tlist entry from the given tlist */ + tle = (TargetEntry *) list_nth(tlist, attrMap[attrno - 1] - 1); + + /* Get the resno right */ + if (tle->resno != attrno) + tle->resno = attrno; + } + else + { + Node *expr; + + Assert(att_tup->attisdropped); + + /* Insert NULL for dropped column */ + expr = (Node *) makeConst(INT4OID, + -1, + InvalidOid, + sizeof(int32), + (Datum) 0, + true, /* isnull */ + true /* byval */ ); + + tle = makeTargetEntry((Expr *) expr, + attrno, + pstrdup(NameStr(att_tup->attname)), + false); + } + + new_tlist = lappend(new_tlist, tle); + } + + return new_tlist; + }