Thread: inserts into partitioned table may cause crash

I've run into what seems to be a bug in ExecInsert() that causes a crash
when inserting multiple rows into a partitioned table that each go into
different partitions with different tuple descriptors.  Crash occurs if
ExecInsert() returns without resetting estate->es_result_relation_info
back to the root table's resultRelInfo.  For example, if a BR trigger on a
partition returns NULL for a row.

Crashing example:

create table p (a int, b text) partition by list (a);
create table p12 (b text, a int);

-- p12 has different tuple descriptor than p
alter table p attach partition p12 for values in (1, 2);

create table p4 partition of p for values in (4);

create function blackhole () returns trigger as $$ begin return NULL; end;
$$ language plpgsql;
create trigger blackhole before insert on p12 for each row execute
procedure blackhole();

insert into p values (1, 'b'), (4, 'a');
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Crash is caused because we enter into ExecFindPartition with p12's
resultRelInfo as if it correponds to the root table.  That happens because
we didn't reset estate->es_result_relation_info, which had been set to
p12's resultRelInfo to point back to the original resultRelInfo (that is,
p's) before returning like below:

       slot = ExecIRInsertTriggers(estate, resultRelInfo, slot);

        if (slot == NULL)       /* "do nothing" */
            return NULL;

There are other places where we prematurely return like that.

Attached a patch to fix that, which would need to be back-patched to 10.

Thanks,
Amit

On 2018/02/28 17:36, Amit Langote wrote:
> I've run into what seems to be a bug in ExecInsert() that causes a crash
> when inserting multiple rows into a partitioned table that each go into
> different partitions with different tuple descriptors.  Crash occurs if
> ExecInsert() returns without resetting estate->es_result_relation_info
> back to the root table's resultRelInfo.  For example, if a BR trigger on a
> partition returns NULL for a row.
> 
> Crashing example:
> 
> create table p (a int, b text) partition by list (a);
> create table p12 (b text, a int);
> 
> -- p12 has different tuple descriptor than p
> alter table p attach partition p12 for values in (1, 2);
> 
> create table p4 partition of p for values in (4);
> 
> create function blackhole () returns trigger as $$ begin return NULL; end;
> $$ language plpgsql;
> create trigger blackhole before insert on p12 for each row execute
> procedure blackhole();
> 
> insert into p values (1, 'b'), (4, 'a');
> server closed the connection unexpectedly
>     This probably means the server terminated abnormally
>     before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> 
> Crash is caused because we enter into ExecFindPartition with p12's
> resultRelInfo as if it correponds to the root table.  That happens because
> we didn't reset estate->es_result_relation_info, which had been set to
> p12's resultRelInfo to point back to the original resultRelInfo (that is,
> p's) before returning like below:
> 
>        slot = ExecIRInsertTriggers(estate, resultRelInfo, slot);
> 
>         if (slot == NULL)       /* "do nothing" */
>             return NULL;
> 
> There are other places where we prematurely return like that.
> 
> Attached a patch to fix that, which would need to be back-patched to 10.

BTW, this won't crash if the table descriptors match, but one would get an
unintuitive error like this:

create table p (a int, b text) partition by list (a);
create table p12 partition of p for values in (1, 2);
create table p4 partition of p for values in (4);
create trigger blackhole before insert on p12 for each row execute
procedure blackhole();

insert into p values (1, 'a'), (4, 'a');
ERROR:  new row for relation "p12" violates partition constraint
DETAIL:  Failing row contains (4, a).

That is, after trying to insert (4, 'a') into p12 as if it were the root.

Still, it's a bug all the same.

Thanks,
Amit

(2018/02/28 17:36), Amit Langote wrote:
> I've run into what seems to be a bug in ExecInsert() that causes a crash
> when inserting multiple rows into a partitioned table that each go into
> different partitions with different tuple descriptors.  Crash occurs if
> ExecInsert() returns without resetting estate->es_result_relation_info
> back to the root table's resultRelInfo.  For example, if a BR trigger on a
> partition returns NULL for a row.
>
> Crashing example:
>
> create table p (a int, b text) partition by list (a);
> create table p12 (b text, a int);
>
> -- p12 has different tuple descriptor than p
> alter table p attach partition p12 for values in (1, 2);
>
> create table p4 partition of p for values in (4);
>
> create function blackhole () returns trigger as $$ begin return NULL; end;
> $$ language plpgsql;
> create trigger blackhole before insert on p12 for each row execute
> procedure blackhole();
>
> insert into p values (1, 'b'), (4, 'a');
> server closed the connection unexpectedly
>     This probably means the server terminated abnormally
>     before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>
> Crash is caused because we enter into ExecFindPartition with p12's
> resultRelInfo as if it correponds to the root table.  That happens because
> we didn't reset estate->es_result_relation_info, which had been set to
> p12's resultRelInfo to point back to the original resultRelInfo (that is,
> p's) before returning like below:
>
>         slot = ExecIRInsertTriggers(estate, resultRelInfo, slot);
>
>          if (slot == NULL)       /* "do nothing" */
>              return NULL;
>
> There are other places where we prematurely return like that.
>
> Attached a patch to fix that, which would need to be back-patched to 10.

Good catch!  Will review.

Best regards,
Etsuro Fujita

(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

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

Hi Amit,

(2018/03/06 15:28), Amit Langote wrote:
> On 2018/03/05 22:00, Etsuro Fujita wrote:
>> 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:

> 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.

Thanks for the review!  I'll leave that for another patch, then.

> 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 for that!

One thing I notice while working on this is this in ExecInsert/CopyFrom, 
which I moved to ExecPrepareTupleRouting as-is for the former:

     /*
      * If we're capturing transition tuples, we might need to convert 
from the
      * partition rowtype to parent rowtype.
      */
     if (mtstate->mt_transition_capture != NULL)
     {
         if (resultRelInfo->ri_TrigDesc &&
             (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
              resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
         {
             /*
              * If there are any BEFORE or INSTEAD triggers on the 
partition,
              * we'll have to be ready to convert their result back to
              * tuplestore format.
              */
             mtstate->mt_transition_capture->tcs_original_insert_tuple = 
NULL;
             mtstate->mt_transition_capture->tcs_map =
                 TupConvMapForLeaf(proute, rootRelInfo, leaf_part_index);
         }

Do we need to consider INSTEAD triggers here?  The partition is either a 
plain table or a foreign table, so I don't think it can have those 
triggers.  Am I missing something?

Best regards,
Etsuro Fujita

(2018/03/06 21:26), Etsuro Fujita wrote:
> One thing I notice while working on this is this in ExecInsert/CopyFrom,
> which I moved to ExecPrepareTupleRouting as-is for the former:
>
> /*
> * If we're capturing transition tuples, we might need to convert from the
> * partition rowtype to parent rowtype.
> */
> if (mtstate->mt_transition_capture != NULL)
> {
> if (resultRelInfo->ri_TrigDesc &&
> (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
> resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
> {
> /*
> * If there are any BEFORE or INSTEAD triggers on the partition,
> * we'll have to be ready to convert their result back to
> * tuplestore format.
> */
> mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL;
> mtstate->mt_transition_capture->tcs_map =
> TupConvMapForLeaf(proute, rootRelInfo, leaf_part_index);
> }
>
> Do we need to consider INSTEAD triggers here? The partition is either a
> plain table or a foreign table, so I don't think it can have those
> triggers. Am I missing something?

There seems to be no objections, so I removed the INSTEAD-trigger 
condition from this.  Here are updated patches for PG10 and HEAD.

Other changes:
* Add regression tests based on your test cases shown upthread
* Adjust code/comments a little bit

Best regards,
Etsuro Fujita

(2018/03/09 20:18), Etsuro Fujita wrote:
> Here are updated patches for PG10 and HEAD.
>
> Other changes:
> * Add regression tests based on your test cases shown upthread

I added a little bit more regression tests and revised comments.  Please 
find attached an updated patch.

Best regards,
Etsuro Fujita

Fujita-san,

Thanks for the updates and sorry I couldn't reply sooner.

On 2018/03/06 21:26, Etsuro Fujita wrote:
> One thing I notice while working on this is this in ExecInsert/CopyFrom,
> which I moved to ExecPrepareTupleRouting as-is for the former:
>
>     /*
>      * If we're capturing transition tuples, we might need to convert from
> the
>      * partition rowtype to parent rowtype.
>      */
>     if (mtstate->mt_transition_capture != NULL)
>     {
>         if (resultRelInfo->ri_TrigDesc &&
>             (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
>              resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
>         {
>             /*
>              * If there are any BEFORE or INSTEAD triggers on the partition,
>              * we'll have to be ready to convert their result back to
>              * tuplestore format.
>              */
>             mtstate->mt_transition_capture->tcs_original_insert_tuple =
NULL;
>             mtstate->mt_transition_capture->tcs_map =
>                 TupConvMapForLeaf(proute, rootRelInfo, leaf_part_index);
>         }
>
> Do we need to consider INSTEAD triggers here?  The partition is either a
> plain table or a foreign table, so I don't think it can have those
> triggers.  Am I missing something?

I think you're right.  We don't need to consider INSTEAD OF triggers here
if we know we're dealing with a partition which cannot have those.

Just to make sure, a word from Thomas would help.

On 2018/03/12 12:25, Etsuro Fujita wrote:
> (2018/03/09 20:18), Etsuro Fujita wrote:
>> Here are updated patches for PG10 and HEAD.
>>
>> Other changes:
>> * Add regression tests based on your test cases shown upthread
> 
> I added a little bit more regression tests and revised comments.  Please
> find attached an updated patch.

Thanks for adding the test.

I was concerned that ExecMaterializeSlot will be called twice now -- first
in ExecPrepareTupleRouting and then in ExecInsert -- instead of only once
in ExecInsert with the existing code, but perhaps it doesn't matter much
because most of the work would be done in the 1st call anyway.

Sorry that this may be nitpicking that I should've brought up before, but
doesn't ExecPrepareTupleRouting do all the work that's needed for routing
a tuple and hence isn't the name a bit misleading?  Maybe,
ExecPerformTupleRouting?

Btw, I noticed that the patches place ExecPrepareTupleRouting (both the
declaration and the definition) at different relative locations within
nodeModifyTable.c in the HEAD branch vs. in the 10 branch.  It might be a
good idea to bring them to the same relative location to avoid hassle when
back-patching relevant patches in the future.  I did that in the attached
updated version (v4) of the patch for HEAD, which does not make any other
changes.  Although, the patch for PG-10 is unchanged, I still changed its
file name to contain v4.

Regards,
Amit

(2018/03/14 14:54), Amit Langote wrote:
> On 2018/03/06 21:26, Etsuro Fujita wrote:
>> One thing I notice while working on this is this in ExecInsert/CopyFrom,

>>      /*
>>       * If we're capturing transition tuples, we might need to convert from
>> the
>>       * partition rowtype to parent rowtype.
>>       */
>>      if (mtstate->mt_transition_capture != NULL)
>>      {
>>          if (resultRelInfo->ri_TrigDesc&&
>>              (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
>>               resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
>>          {
>>              /*
>>               * If there are any BEFORE or INSTEAD triggers on the partition,
>>               * we'll have to be ready to convert their result back to
>>               * tuplestore format.
>>               */
>>              mtstate->mt_transition_capture->tcs_original_insert_tuple =
> NULL;
>>              mtstate->mt_transition_capture->tcs_map =
>>                  TupConvMapForLeaf(proute, rootRelInfo, leaf_part_index);
>>          }
>>
>> Do we need to consider INSTEAD triggers here?  The partition is either a
>> plain table or a foreign table, so I don't think it can have those
>> triggers.  Am I missing something?
>
> I think you're right.  We don't need to consider INSTEAD OF triggers here
> if we know we're dealing with a partition which cannot have those.
>
> Just to make sure, a word from Thomas would help.

+1

> I was concerned that ExecMaterializeSlot will be called twice now -- first
> in ExecPrepareTupleRouting and then in ExecInsert -- instead of only once
> in ExecInsert with the existing code, but perhaps it doesn't matter much
> because most of the work would be done in the 1st call anyway.

Yeah, if we force the slot into the materialized state in the 1st call, 
then we won't do that in the 2nd call.

> Sorry that this may be nitpicking that I should've brought up before, but
> doesn't ExecPrepareTupleRouting do all the work that's needed for routing
> a tuple and hence isn't the name a bit misleading?  Maybe,
> ExecPerformTupleRouting?

Actually, I thought of that name as a candidate, too.  But I used 
ExecPrepareTupleRouting because I didn't think it would actually perform 
all the work; because it wouldn't do the main work of routing, ie, route 
an inserted tuple to the target partition, which is ExecInsert()'s job. 
  I agree that it would do all the work *needed for routing*, though.

> Btw, I noticed that the patches place ExecPrepareTupleRouting (both the
> declaration and the definition) at different relative locations within
> nodeModifyTable.c in the HEAD branch vs. in the 10 branch.  It might be a
> good idea to bring them to the same relative location to avoid hassle when
> back-patching relevant patches in the future.  I did that in the attached
> updated version (v4) of the patch for HEAD, which does not make any other
> changes.  Although, the patch for PG-10 is unchanged, I still changed its
> file name to contain v4.

That's a good idea!  Thanks for the updated patches!

Best regards,
Etsuro Fujita

(2018/03/14 17:25), Etsuro Fujita wrote:
> (2018/03/14 14:54), Amit Langote wrote:
>> Sorry that this may be nitpicking that I should've brought up before, but
>> doesn't ExecPrepareTupleRouting do all the work that's needed for routing
>> a tuple and hence isn't the name a bit misleading? Maybe,
>> ExecPerformTupleRouting?
>
> Actually, I thought of that name as a candidate, too. But I used
> ExecPrepareTupleRouting because I didn't think it would actually perform
> all the work; because it wouldn't do the main work of routing, ie, route
> an inserted tuple to the target partition, which is ExecInsert()'s job.
> I agree that it would do all the work *needed for routing*, though.

One thing to add: having said that, I don't have any strong opinion 
about that.  How about leaving that for the committer?

Best regards,
Etsuro Fujita

On 2018/03/14 17:35, Etsuro Fujita wrote:
> (2018/03/14 17:25), Etsuro Fujita wrote:
>> (2018/03/14 14:54), Amit Langote wrote:
>>> Sorry that this may be nitpicking that I should've brought up before, but
>>> doesn't ExecPrepareTupleRouting do all the work that's needed for routing
>>> a tuple and hence isn't the name a bit misleading? Maybe,
>>> ExecPerformTupleRouting?
>>
>> Actually, I thought of that name as a candidate, too. But I used
>> ExecPrepareTupleRouting because I didn't think it would actually perform
>> all the work; because it wouldn't do the main work of routing, ie, route
>> an inserted tuple to the target partition, which is ExecInsert()'s job.
>> I agree that it would do all the work *needed for routing*, though.
> 
> One thing to add: having said that, I don't have any strong opinion about
> that.  How about leaving that for the committer?

Sure.  I agree with your point that "routing" isn't finished in that
function if we also consider actual insertion of tuple into the selected
partition a part of it.

Thanks,
Amit

On Wed, Mar 14, 2018 at 9:25 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> (2018/03/14 14:54), Amit Langote wrote:
>> On 2018/03/06 21:26, Etsuro Fujita wrote:
>>>      if (mtstate->mt_transition_capture != NULL)
>>>      {
>>>          if (resultRelInfo->ri_TrigDesc&&
>>>              (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
>>>               resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
>>>          {
>>>              /*
>>>               * If there are any BEFORE or INSTEAD triggers on the partition,
>>>               * we'll have to be ready to convert their result back to
>>>               * tuplestore format.
>>>               */
>>>              mtstate->mt_transition_capture->tcs_original_insert_tuple =
>>
>> NULL;
>>>
>>>              mtstate->mt_transition_capture->tcs_map =
>>>                  TupConvMapForLeaf(proute, rootRelInfo, leaf_part_index);
>>>          }
>>>
>>> Do we need to consider INSTEAD triggers here?  The partition is either a
>>> plain table or a foreign table, so I don't think it can have those
>>> triggers.  Am I missing something?
>>
>> I think you're right.  We don't need to consider INSTEAD OF triggers here
>> if we know we're dealing with a partition which cannot have those.
>>
>> Just to make sure, a word from Thomas would help.
>
> +1

Agreed.  INSTEAD OF triggers can only be created on views, and views
can't appear in a partition hierarchy, so the comment is misleading
and the test is redundant.  Thanks for picking this up.

-- 
Thomas Munro
http://www.enterprisedb.com

(2018/03/14 17:25), Etsuro Fujita wrote:
> (2018/03/14 14:54), Amit Langote wrote:
>> Btw, I noticed that the patches place ExecPrepareTupleRouting (both the
>> declaration and the definition) at different relative locations within
>> nodeModifyTable.c in the HEAD branch vs. in the 10 branch. It might be a
>> good idea to bring them to the same relative location to avoid hassle
>> when
>> back-patching relevant patches in the future. I did that in the attached
>> updated version (v4) of the patch for HEAD, which does not make any other
>> changes. Although, the patch for PG-10 is unchanged, I still changed its
>> file name to contain v4.
>
> That's a good idea! Thanks for the updated patches!

Sorry, I didn't look at those patches carefully, but I noticed that 
while the patch for PG10 has put the definition of that function after 
the definitions of functions such as fireBSTriggers, fireASTriggers and 
ExecSetupTransitionCaptureState, but the patch for HEAD puts it *before* 
the definitions of these functions (note: these functions don't have 
their declarations at the file header), which seems a bit inconsistent. 
  ISTM that the v3 patches for PG10 and HEAD have already put that 
function in the right place in terms of that relativity.

Best regards,
Etsuro Fujita

Fujita-san,

Thanks for the review.

On 2018/03/16 20:37, Etsuro Fujita wrote:
> (2018/03/14 17:25), Etsuro Fujita wrote:
>> (2018/03/14 14:54), Amit Langote wrote:
>>> Btw, I noticed that the patches place ExecPrepareTupleRouting (both the
>>> declaration and the definition) at different relative locations within
>>> nodeModifyTable.c in the HEAD branch vs. in the 10 branch. It might be a
>>> good idea to bring them to the same relative location to avoid hassle
>>> when
>>> back-patching relevant patches in the future. I did that in the attached
>>> updated version (v4) of the patch for HEAD, which does not make any other
>>> changes. Although, the patch for PG-10 is unchanged, I still changed its
>>> file name to contain v4.
>>
>> That's a good idea! Thanks for the updated patches!
> 
> Sorry, I didn't look at those patches carefully, but I noticed that while
> the patch for PG10 has put the definition of that function after the
> definitions of functions such as fireBSTriggers, fireASTriggers and
> ExecSetupTransitionCaptureState, but the patch for HEAD puts it *before*
> the definitions of these functions (note: these functions don't have their
> declarations at the file header), which seems a bit inconsistent.  ISTM
> that the v3 patches for PG10 and HEAD have already put that function in
> the right place in terms of that relativity.

That's correct, except v3 had added the definition of
ExecPrepareTupleRouting after those of ExecSetupChildParentMapForSubplan,
ExecSetupChildParentMapForTcs, etc., that are new in 11dev branch.

I've fixed the patch for HEAD to move the ExecPrepareTupleRouting
definition to appear after those of fireBSTriggers, fireASTriggers, and
ExecSetupTransitionCaptureState.

Attached are v5 patches for HEAD and 10 branches.

Thanks,
Amit

(2018/03/19 17:48), Amit Langote wrote:
> On 2018/03/16 20:37, Etsuro Fujita wrote:
>> (2018/03/14 17:25), Etsuro Fujita wrote:
>>> (2018/03/14 14:54), Amit Langote wrote:
>>>> Btw, I noticed that the patches place ExecPrepareTupleRouting (both the
>>>> declaration and the definition) at different relative locations within
>>>> nodeModifyTable.c in the HEAD branch vs. in the 10 branch. It might be a
>>>> good idea to bring them to the same relative location to avoid hassle
>>>> when
>>>> back-patching relevant patches in the future. I did that in the attached
>>>> updated version (v4) of the patch for HEAD, which does not make any other
>>>> changes. Although, the patch for PG-10 is unchanged, I still changed its
>>>> file name to contain v4.
>>>
>>> That's a good idea! Thanks for the updated patches!
>>
>> Sorry, I didn't look at those patches carefully, but I noticed that while
>> the patch for PG10 has put the definition of that function after the
>> definitions of functions such as fireBSTriggers, fireASTriggers and
>> ExecSetupTransitionCaptureState, but the patch for HEAD puts it *before*
>> the definitions of these functions (note: these functions don't have their
>> declarations at the file header), which seems a bit inconsistent.  ISTM
>> that the v3 patches for PG10 and HEAD have already put that function in
>> the right place in terms of that relativity.
>
> That's correct, except v3 had added the definition of
> ExecPrepareTupleRouting after those of ExecSetupChildParentMapForSubplan,
> ExecSetupChildParentMapForTcs, etc., that are new in 11dev branch.
>
> I've fixed the patch for HEAD to move the ExecPrepareTupleRouting
> definition to appear after those of fireBSTriggers, fireASTriggers, and
> ExecSetupTransitionCaptureState.
>
> Attached are v5 patches for HEAD and 10 branches.

Thanks for the updated patches!  I think the patches are in good shape, 
but I did a bit of editorial things; added a bit more comments for 
ExecPrepareTupleRouting and adjusted regression test stuff to match the 
existing ones.  Attached are the updated patches for HEAD and PG10. 
Those changes are just editorial, so let's ask for the committer review.

Best regards,
Etsuro Fujita

Etsuro Fujita wrote:

> Thanks for the updated patches!  I think the patches are in good shape, but
> I did a bit of editorial things; added a bit more comments for
> ExecPrepareTupleRouting and adjusted regression test stuff to match the
> existing ones.  Attached are the updated patches for HEAD and PG10. Those
> changes are just editorial, so let's ask for the committer review.

Thanks, I'll give these a look now.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Mar 19, 2018 at 9:38 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Etsuro Fujita wrote:
>> Thanks for the updated patches!  I think the patches are in good shape, but
>> I did a bit of editorial things; added a bit more comments for
>> ExecPrepareTupleRouting and adjusted regression test stuff to match the
>> existing ones.  Attached are the updated patches for HEAD and PG10. Those
>> changes are just editorial, so let's ask for the committer review.
>
> Thanks, I'll give these a look now.

Thanks!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Etsuro Fujita wrote:

> Thanks for the updated patches!  I think the patches are in good shape, but
> I did a bit of editorial things; added a bit more comments for
> ExecPrepareTupleRouting and adjusted regression test stuff to match the
> existing ones.  Attached are the updated patches for HEAD and PG10. Those
> changes are just editorial, so let's ask for the committer review.

Thanks, looks good!  I made another pass over the comments (a few looked
too much like they were in a larger function) and pushed to both
branches.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018/03/20 5:54, Alvaro Herrera wrote:
> Etsuro Fujita wrote:
> 
>> Thanks for the updated patches!  I think the patches are in good shape, but
>> I did a bit of editorial things; added a bit more comments for
>> ExecPrepareTupleRouting and adjusted regression test stuff to match the
>> existing ones.  Attached are the updated patches for HEAD and PG10. Those
>> changes are just editorial, so let's ask for the committer review.
> 
> Thanks, looks good!  I made another pass over the comments (a few looked
> too much like they were in a larger function) and pushed to both
> branches.

Thank you!

Regards,
Amit

(2018/03/20 9:34), Amit Langote wrote:
> On 2018/03/20 5:54, Alvaro Herrera wrote:
>> Etsuro Fujita wrote:
>>
>>> Thanks for the updated patches!  I think the patches are in good shape, but
>>> I did a bit of editorial things; added a bit more comments for
>>> ExecPrepareTupleRouting and adjusted regression test stuff to match the
>>> existing ones.  Attached are the updated patches for HEAD and PG10. Those
>>> changes are just editorial, so let's ask for the committer review.
>>
>> Thanks, looks good!  I made another pass over the comments (a few looked
>> too much like they were in a larger function) and pushed to both
>> branches.
>
> Thank you!

Thanks, Alvaro and Amit!

Best regards,
Etsuro Fujita