Fujita-san,
Thanks for the review.
On 2018/03/05 22:00, Etsuro Fujita wrote:
> I started reviewing this. I think the analysis you mentioned upthread
> would be correct, but I'm not sure the patch is the right way to go
> because I think that exception handling added by the patch throughout
> ExecInsert such as:
>
> @@ -408,7 +408,10 @@ ExecInsert(ModifyTableState *mtstate,
> slot = ExecBRInsertTriggers(estate, resultRelInfo, slot);
>
> if (slot == NULL) /* "do nothing" */
> - return NULL;
> + {
> + result = NULL;
> + goto restore_estate_result_rel;
> + }
>
> would reduce the code readability.
Hmm yeah, it is a bit hacky.
> An alternative fix for this would be to handle the set/reset of
> estate->es_result_relation_info in a higher level ie, ExecModifyTable,
> like the attached: (1) before calling ExecInsert, we do the preparation
> work for tuple routing of one tuple (eg, determine the partition for the
> tuple and convert the format of the tuple in a separate function, which
> also sets estate->es_result_relation_info to the partition for ExecInsert
> to work on it; then (2) we call ExecInsert, which just inserts into the
> partition; then (3) we reset estate->es_result_relation_info back to the
> root partitioned table. One good thing about the alternative is to return
> ExecInsert somehow to PG9.6, which would make maintenance easier. COPY
> has the same issue, so the attached also fixes that. (Maybe we could do
> some refactoring to use the same code in both cases, but I'm not sure we
> should do that as a fix.) What do you think about the alternative?
Your patch seems like a good cleanup overall, fixes this bug, and seems to
make future maintenance easier. So, +1. I agree that doing a surgery
like this on COPY is better left for later.
I noticed (as you may have too) that it cannot be applied to the 10 branch
as is. I made the necessary changes so that it can be. See attached
patch with filename suffixed "-10backpatch". Also attached is your patch
unchanged to have both of them together.
Thanks,
Amit
