Amit Langote wrote: > Actually, after your comment on my original patch [1], I did make it work > for multiple levels by teaching the partition initialization code to find > a given partition's indexes that are inherited from the root table (that > is the table mentioned in command). So, after a tuple is routed to a > partition, we switch from the original arbiterIndexes list to the one we > created for the partition, which must contain OIDs corresponding to those > in the original list. After all, for each of the parent's indexes that > the planner put into the original arbiterIndexes list, there must exist an > index in each of the leaf partitions.
Oh, your solution for this seems simple enough. Silly me, I was trying to implement it in a quite roundabout way. Thanks. (I do wonder if we should save the "root" reloid in the relcache). > I had also observed when working on the patch that various TupleTableSlots > used by the ON CONFLICT DO UPDATE code must be based on TupleDesc of the > inheritance-translated target list (DO UPDATE SET target list). In fact, > that has to take into account also the dropped columns; we may have > dropped columns either in parent or in a partition or in both at same or > different attnum positions. That means, simple map_partition_varattnos() > translation doesn't help in this case. Yeah, I was aware these corner cases could become a problem though I hadn't gotten around to testing them yet. Thanks for all your work on this. The usage of the few optimizer/prep/ functions that are currently static doesn't fill me with joy. These functions have weird APIs because they're static so we don't rally care, but once we export them we should strive to be more careful. I'd rather stay away from just exporting them all, so I chose to encapsulate those calls in a single function and export only expand_targetlist from preptlist.c, keeping the others static in prepunion.c. In the attached patch set, I put an API change (work on tupdescs rather than full-blown relations) for a couple of those functions as 0001, then your patch as 0002, then a few fixups of my own. (0002 is not bit-by-bit identical to yours; I think I had to fix some merge conflict with 0001, but should be pretty much the same). But looking further, I think there is much cruft that has accumulated in those functions (because they've gotten simplified over time), and we could do some additional cleanup surgery. For example, there is no reason to pass a list pointer to make_inh_translation_list(); we could just return it. And then we don't have to cons up a fake AppendRelInfo with all dummy values that adjust_inherited_tlist doesn't even care about. I think there was a point for all these contortions back at some point (visible by checking git history of this code), but it all seems useless now. 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? 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. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
