Re: inserts into partitioned table may cause crash

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