Pushed now.
Amit Langote wrote:
> On 2018/03/24 9:23, Alvaro Herrera wrote:
> > To fix this, I had to completely rework the "get partition parent root"
> > stuff into "get list of ancestors of this partition".
>
> I wondered if a is_partition_ancestor(partrelid, ancestorid) isn't enough
> instead of creating a list of ancestors and then looping over it as you've
> done, but maybe what you have here is fine.
Yeah, I wondered about doing it that way too (since you can stop looking
early), but decided that I didn't like repeatedly opening pg_inherits
for each level. Anyway the most common case is a single level, and in
rare cases two levels ... I don't think we're going to see much more
than that. So it doesn't matter too much. We can refine later anyway,
if this becomes a hot spot (I doubt it TBH).
> > * General code style improvements, comment rewording, etc.
>
> There was one comment in Fujita-san's review he posted on Friday [1] that
> doesn't seem to be addressed in v10, which I think we probably should. It
> was this comment:
>
> "ExecBuildProjectionInfo is called without setting the tuple descriptor of
> mtstate->mt_conflproj to tupDesc. That might work at least for now, but I
> think it's a good thing to set it appropriately to make that future proof."
>
> All of his other comments seem to have been taken care of in v10. I have
> fixed the above one in the attached updated version.
I was of two minds about this item myself; we don't use the tupdesc for
anything at that point and I expect more things would break if we
required that. But I don't think it hurts, so I kept it.
The one thing I wasn't terribly in love with is the four calls to
map_partition_varattnos(), creating the attribute map four times ... but
we already have it in the TupleConversionMap, no? Looks like we could
save a bunch of work there.
And a final item is: can we have a whole-row expression in the clauses?
We currently don't handle those either, not even to throw an error.
[figures a test case] ... and now that I test it, it does crash!
create table part (a int primary key, b text) partition by range (a);
create table part1 (b text, a int not null);
alter table part attach partition part1 for values from (1) to (1000);
insert into part values (1, 'two') on conflict (a)
do update set b = format('%s (was %s)', excluded.b, part.b)
where part.* *<> (1, text 'two');
I think this means we should definitely handle found_whole_row. (If you
create part1 in the normal way, it works as you'd expect.)
I'm going to close a few other things first, then come back to this.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
