Thread: Multi-insert into a partitioned table with before insert row triggercauses server crash on latest HEAD

Hi All,

The copy command on partitioned table causes server crash when before
insert row trigger is created on one of its partition. Please find the
following test-case  to reproduce the crash.

-- create a partitioned table
create table part_copy_test  (a int, b int, c text) partition by list (b);
create table part_copy_test_a1 partition of part_copy_test for values in(1);
create table part_copy_test_a2 partition of part_copy_test for values in(2);

-- insert enough rows to allow multi-insert into a partitioned table.
copy (select x,1,'One' from generate_series(1,1000) x
union all
select x,2,'two' from generate_series(1001,1010) x
union all
select x,1,'One' from generate_series(1011,1020) x) to
'/tmp/multi_insert_data.csv';

-- create before insert row trigger on part_copy_test_a2
create function part_ins_func() returns trigger language plpgsql as
$$
begin
  return new;
end;
$$;

create trigger part_ins_trig before insert on part_copy_test_a2
for each row execute procedure part_ins_func();

-- run copy command on partitioned table.
copy part_copy_test from '/tmp/multi_insert_data.csv';

postgres=# copy part_copy_test from '/tmp/multi_insert_data.csv';
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.
!>

I've spent some time looking into this issue and found that,
CopyFrom() is trying perform multi-insert for the partition that has
before insert row trigger created on it which is not expected. When a
normal table with before insert row trigger is created, CopyFrom
doesn't allow multi insert on such tables and i guess same should be
the case with table partition as well. Please correct me if i my
understanding is wrong ?

Thanks,

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Hi,

On 2018-10-12 21:16:25 +0530, Ashutosh Sharma wrote:
> The copy command on partitioned table causes server crash when before
> insert row trigger is created on one of its partition. Please find the
> following test-case  to reproduce the crash.
> 
> -- create a partitioned table
> create table part_copy_test  (a int, b int, c text) partition by list (b);
> create table part_copy_test_a1 partition of part_copy_test for values in(1);
> create table part_copy_test_a2 partition of part_copy_test for values in(2);
> 
> -- insert enough rows to allow multi-insert into a partitioned table.
> copy (select x,1,'One' from generate_series(1,1000) x
> union all
> select x,2,'two' from generate_series(1001,1010) x
> union all
> select x,1,'One' from generate_series(1011,1020) x) to
> '/tmp/multi_insert_data.csv';
> 
> -- create before insert row trigger on part_copy_test_a2
> create function part_ins_func() returns trigger language plpgsql as
> $$
> begin
>   return new;
> end;
> $$;
> 
> create trigger part_ins_trig before insert on part_copy_test_a2
> for each row execute procedure part_ins_func();
> 
> -- run copy command on partitioned table.
> copy part_copy_test from '/tmp/multi_insert_data.csv';
> 
> postgres=# copy part_copy_test from '/tmp/multi_insert_data.csv';
> 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.
> !>
> 
> I've spent some time looking into this issue and found that,
> CopyFrom() is trying perform multi-insert for the partition that has
> before insert row trigger created on it which is not expected. When a
> normal table with before insert row trigger is created, CopyFrom
> doesn't allow multi insert on such tables and i guess same should be
> the case with table partition as well. Please correct me if i my
> understanding is wrong ?

Yea, that anlysis sounds right. Peter?

Greetings,

Andres Freund

>
> Yea, that anlysis sounds right. Peter?
>
Thanks Andres for the confirmation on my initial analysis. I'm not
sure why Peter hasn't commented on it so far. However, I continued
investigating on it further and here are findings

I think, the root cause of this problem is that CopyFrom() is using
the stale value of *has_before_insert_row_trig* to determine if the
current partition is okay for multi-insert or not i.e.
has_before_insert_row_trig used to determine multi-insert condition
for the current partition actually belongs to old partition. I think,
*has_before_insert_row_trig* needs to updated before CopyFrom()
evaluates if the current partition is good to go for multi insert or
not. Attached is the patch based on this. I've also added the relevant
test-case for it. Peter, David, Could you please have a look into the
attached patch and share your thoughts. Thank you.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On 16/10/2018 06:33, Ashutosh Sharma wrote:
> I think, the root cause of this problem is that CopyFrom() is using
> the stale value of *has_before_insert_row_trig* to determine if the
> current partition is okay for multi-insert or not i.e.
> has_before_insert_row_trig used to determine multi-insert condition
> for the current partition actually belongs to old partition. I think,
> *has_before_insert_row_trig* needs to updated before CopyFrom()
> evaluates if the current partition is good to go for multi insert or
> not. Attached is the patch based on this. I've also added the relevant
> test-case for it. Peter, David, Could you please have a look into the
> attached patch and share your thoughts. Thank you.

I have committed your fix and test, moving some code around a bit.  Thanks.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Oct 18, 2018 at 12:15 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> On 16/10/2018 06:33, Ashutosh Sharma wrote:
> > I think, the root cause of this problem is that CopyFrom() is using
> > the stale value of *has_before_insert_row_trig* to determine if the
> > current partition is okay for multi-insert or not i.e.
> > has_before_insert_row_trig used to determine multi-insert condition
> > for the current partition actually belongs to old partition. I think,
> > *has_before_insert_row_trig* needs to updated before CopyFrom()
> > evaluates if the current partition is good to go for multi insert or
> > not. Attached is the patch based on this. I've also added the relevant
> > test-case for it. Peter, David, Could you please have a look into the
> > attached patch and share your thoughts. Thank you.
>
> I have committed your fix and test, moving some code around a bit.  Thanks.
>

Thanks Peter.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

(Returns from leave and beyond the reaches of the internet)

On 18 October 2018 at 07:45, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 16/10/2018 06:33, Ashutosh Sharma wrote:
>> I think, the root cause of this problem is that CopyFrom() is using
>> the stale value of *has_before_insert_row_trig* to determine if the
>> current partition is okay for multi-insert or not i.e.
>> has_before_insert_row_trig used to determine multi-insert condition
>> for the current partition actually belongs to old partition. I think,
>> *has_before_insert_row_trig* needs to updated before CopyFrom()
>> evaluates if the current partition is good to go for multi insert or
>> not. Attached is the patch based on this. I've also added the relevant
>> test-case for it. Peter, David, Could you please have a look into the
>> attached patch and share your thoughts. Thank you.
>
> I have committed your fix and test, moving some code around a bit.  Thanks.

Thanks for pushing that fix.

Originally my patch in [1] only could set leafpart_use_multi_insert to
true within the `if (insertMethod == CIM_MULTI_CONDITIONAL)` test, so
wouldn't have suffered from this problem.

I'm not sure that doubling up the `insertMethod ==
CIM_MULTI_CONDITIONAL` test is the cleanest fix. Personally, I liked
the way it was in the v6 edition of the patch, but I'm used to getting
outvoted.

[1] https://www.postgresql.org/message-id/CAKJS1f9f8yuj04X_rffNu2JPbvhy+YP_aVH6iwCTJ1OL=YwCOg@mail.gmail.com

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

> Thanks for pushing that fix.
>
> Originally my patch in [1] only could set leafpart_use_multi_insert to
> true within the `if (insertMethod == CIM_MULTI_CONDITIONAL)` test, so
> wouldn't have suffered from this problem.
>

The problem is reproducible even with the patch in [1] combined with
the v6 edition of delta patch. Actually this problem has nothing to do
with where we set leafpart_use_multi_insert flag. It's about using the
right values (updated values) to decide if the current partition is
good to go for multi-insert or not. Like i mentioned earlier reply,
we actually came across this problem because we have used outdated
value for before insert row trigger to evaluate
leafpart_use_multi_insert flag.

 [1] https://www.postgresql.org/message-id/CAKJS1f9f8yuj04X_rffNu2JPbvhy+YP_aVH6iwCTJ1OL=YwCOg@mail.gmail.com

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On 21 October 2018 at 04:03, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>> Thanks for pushing that fix.
>>
>> Originally my patch in [1] only could set leafpart_use_multi_insert to
>> true within the `if (insertMethod == CIM_MULTI_CONDITIONAL)` test, so
>> wouldn't have suffered from this problem.
>>
>
> The problem is reproducible even with the patch in [1] combined with
> the v6 edition of delta patch. Actually this problem has nothing to do
> with where we set leafpart_use_multi_insert flag. It's about using the
> right values (updated values) to decide if the current partition is
> good to go for multi-insert or not. Like i mentioned earlier reply,
> we actually came across this problem because we have used outdated
> value for before insert row trigger to evaluate
> leafpart_use_multi_insert flag.
>
>  [1] https://www.postgresql.org/message-id/CAKJS1f9f8yuj04X_rffNu2JPbvhy+YP_aVH6iwCTJ1OL=YwCOg@mail.gmail.com

Oops. I was completely wrong about all of what I said above. Seems my
analysis was rushed and very incorrect.

Thanks for writing the patch Ashutosh and thanks Peter for committing
the modified version of it.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services