Re: inserts into partitioned table may cause crash

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: inserts into partitioned table may cause crash
Date: 2018-03-06 06:28:21
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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.


Attachment Content-Type Size
ExecInsert-reset-estate-result-rel-efujita.patch text/plain 12.4 KB
ExecInsert-reset-estate-result-rel-efujita-10backpatch.patch text/plain 9.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2018-03-06 07:21:24 Re: STATISTICS retained in CREATE TABLE ... LIKE (INCLUDING ALL)?
Previous Message Pavel Stehule 2018-03-06 06:14:00 Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs