On 2018/03/24 9:23, Alvaro Herrera wrote:
> I made a bunch of further edits and I think this v10 is ready to push.
> Before doing so I'll give it a final look, particularly because of the
> new elog(ERROR) I added. Post-commit review is of course always
> appreciated.
>
> Most notable change is because I noticed that if you mention an
> intermediate partition level in the INSERT command, and the index is on
> the top level, arbiter index selection fails to find the correct index
> because it walks all the way to the top instead of stopping in the
> middle, as it should (the command was still working because it ended up
> with an empty arbiter index list).
Good catch!
> 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.
> Because of this, I added a new check that the partition's arbiter index
> list is same length as parent's; if not, throw an error. I couldn't get
> it to fire (so it's just an elog not ereport), but maybe I just didn't
> try any weird enough scenarios.
>
> Other changes:
>
> * I added a copyObject() call for nodes we're operating upon. Maybe
> this is unnecessary but the comments claimed "we're working on a copy"
> and I couldn't find any place where we were actually making one.
> Anyway it seems sane to make a copy, because we're scribbling on those
> nodes ... I hope I didn't introduce any serious memory leaks.
That seems fine as ExecInitPartitionInfo allocates in the query context
(es_query_cxt).
> * I made the new OnConflictSetState thing into a proper node. Like
> ResultRelInfo, it doesn't have any niceties like nodeToString support,
> but it seems saner this way (palloc -> makeNode). I reworked the
> formatting of that struct definition too, and renamed members.
Looks good, thanks.
> * I removed an assertion block at the bottom of adjust_partition_tlist.
> It seemed quite pointless, since it was just about checking that the
> resno values were sorted, but by construction we already know that
> they are indeed sorted ...
Hmm yes.
> * 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.
Thanks,
Amit
[1] https://www.postgresql.org/message-id/5AB4DEB6.2020901%40lab.ntt.co.jp
