On 2018/03/05 18:04, Pavan Deolasee wrote: > On Fri, Mar 2, 2018 at 9:06 PM, Alvaro Herrera <alvhe...@2ndquadrant.com> > wrote: >> Re. the "ugly hack" comments in adjust_inherited_tlist(), I'm confused: >> expand_targetlist() runs *after*, not before, so how could it have >> affected the result? >> > > If I understand correctly, planner must have called expand_targetlist() > once for the parent relation's descriptor and added any dropped columns > from the parent relation. So we may not find mapped attributes for those > dropped columns in the parent. I haven't actually tested this case though. > > I wonder if it makes sense to actually avoid expanding on-conflict-set > targetlist in case the target is a partition table and deal with it during > execution, like we are doing now. >> I'm obviously confused about what >> expand_targetlist call this comment is talking about. Anyway I wanted >> to make it use resjunk entries instead, but that broke some other case >> that I didn't have time to research yesterday. I'll get back to this >> soon, but in the meantime, here's what I have. >> > > + conv_setproj = > + > adjust_and_expand_partition_tlist(RelationGetDescr(firstResultRel), > + RelationGetDescr(partrel), > + > RelationGetRelationName(partrel), > + firstVarno, > + conv_setproj); > > Aren't we are adding back Vars referring to the parent relation, after > having converted the existing ones to use partition relation's varno? May > be that works because missing attributes are already added during planning > and expand_targetlist() here only adds dropped columns, which are just NULL > constants.
I think this suggestion to defer onConflictSet target list expansion to execution time is a good one. So, in preprocess_targetlist(), we'd only perform exapand_targetlist on the onConflictSet list if the table is not a partitioned table. For partitioned tables, we don't know which partition(s) will be affected, so it's useless to do the expansion there. Instead it's better to expand in ExecInitPartitionInfo(), possibly after changing original TargetEntry nodes to have correct resnos due to attribute number differences (calling adjust_inherited_tlist(), etc.). And then since we're not expanding the target list in the planner anymore, we don't need to install those hacks in adjust_inherited_tlist() like the patch currently does to ignore entries for dropped columns in the parent the plan-time expansion adds. Thanks, Amit
