Re: inserts into partitioned table may cause crash

(2018/03/01 21:40), Etsuro Fujita wrote:
> (2018/02/28 17:36), Amit Langote wrote:
>> Attached a patch to fix that, which would need to be back-patched to 10.
>
> Good catch! Will review.

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.

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?

Best regards,
Etsuro Fujita