Thread: BUG #15437: Segfault during insert into declarative partitioned tablewith a trigger creating partition

The following bug has been logged on the website:

Bug reference:      15437
Logged by:          Dmitry Shalashov
Email address:      skaurus@gmail.com
PostgreSQL version: 10.5
Operating system:   Debian 9
Description:

I tried to use declarative partitioning and, to avoid creating partitions by
hand, to make them in ON BEFORE STATEMENT trigger. Trigger executes
successfully (proved that with RAISE NOTICE), but server crashes.

Error in the log could look like that:

[24329207.147193] postgres[23599]: segfault at 15948 ip 00005652ff2e586e sp
00007fffd9ee5a50 error 4 in postgres[5652ff17d000+6de000]
2018-10-18 14:52:13 MSK [4312]: [17-1] user=,db= LOG:  server process (PID
4636) was terminated by signal 11: Segmentation fault
2018-10-18 14:52:13 MSK [4312]: [18-1] user=,db= DETAIL:  Failed process was
running: INSERT INTO test (value) VALUES (1);
2018-10-18 14:52:13 MSK [4312]: [19-1] user=,db= LOG:  terminating any other
active server processes

Test case follows:

CREATE TABLE test (id serial NOT NULL, value integer NOT NULL, ctime
timestamp NOT NULL DEFAULT now()) PARTITION BY RANGE (ctime);

CREATE OR REPLACE FUNCTION week_table_suffix_from_ts(ts timestamp with time
zone) RETURNS text
    LANGUAGE plpgsql
    AS $$
DECLARE
    table_suffix text := replace(substring((date_trunc('week', ts))::text
from 1 for 10),'-','');
BEGIN
    RETURN table_suffix;
END;
$$;

CREATE OR REPLACE FUNCTION test_trigger_func() RETURNS trigger
    LANGUAGE plpgsql
    AS $$
DECLARE
    table_prefix    text    := 'test_';
    table_suffix    text    := week_table_suffix_from_ts(now());
    table_name      text    := 'creatives_' || table_suffix;
    table_exists    boolean;
BEGIN
    EXECUTE format(
        'SELECT EXISTS (SELECT 1 FROM pg_tables WHERE schemaname = $1 AND
tablename = $2)'
    ) INTO table_exists USING 'public', table_name;

    IF NOT table_exists THEN
        EXECUTE format(
            'CREATE TABLE IF NOT EXISTS %I PARTITION OF test FOR VALUES FROM
(%L) TO (%L)'::text,
            table_name,
            date_trunc('week', now()),
            date_trunc('week', now() + interval '1 week')
        );
    END IF;

    RETURN NULL;
END;
$$;

CREATE TRIGGER test_trigger BEFORE INSERT ON test FOR EACH STATEMENT EXECUTE
PROCEDURE test_trigger_func();

INSERT INTO test (value) VALUES (1);

On 2018/10/18 20:57, PG Bug reporting form wrote:
> The following bug has been logged on the website:
> 
> Bug reference:      15437
> Logged by:          Dmitry Shalashov
> Email address:      skaurus@gmail.com
> PostgreSQL version: 10.5
> Operating system:   Debian 9
> Description:        
> 
> I tried to use declarative partitioning and, to avoid creating partitions by
> hand, to make them in ON BEFORE STATEMENT trigger. Trigger executes
> successfully (proved that with RAISE NOTICE), but server crashes.
> 
> Error in the log could look like that:
> 
> [24329207.147193] postgres[23599]: segfault at 15948 ip 00005652ff2e586e sp
> 00007fffd9ee5a50 error 4 in postgres[5652ff17d000+6de000]
> 2018-10-18 14:52:13 MSK [4312]: [17-1] user=,db= LOG:  server process (PID
> 4636) was terminated by signal 11: Segmentation fault
> 2018-10-18 14:52:13 MSK [4312]: [18-1] user=,db= DETAIL:  Failed process was
> running: INSERT INTO test (value) VALUES (1);
> 2018-10-18 14:52:13 MSK [4312]: [19-1] user=,db= LOG:  terminating any other
> active server processes
> 
> Test case follows:
> 
> CREATE TABLE test (id serial NOT NULL, value integer NOT NULL, ctime
> timestamp NOT NULL DEFAULT now()) PARTITION BY RANGE (ctime);
> 
> CREATE OR REPLACE FUNCTION week_table_suffix_from_ts(ts timestamp with time
> zone) RETURNS text
>     LANGUAGE plpgsql
>     AS $$
> DECLARE
>     table_suffix text := replace(substring((date_trunc('week', ts))::text
> from 1 for 10),'-','');
> BEGIN
>     RETURN table_suffix;
> END;
> $$;
> 
> CREATE OR REPLACE FUNCTION test_trigger_func() RETURNS trigger
>     LANGUAGE plpgsql
>     AS $$
> DECLARE
>     table_prefix    text    := 'test_';
>     table_suffix    text    := week_table_suffix_from_ts(now());
>     table_name      text    := 'creatives_' || table_suffix;
>     table_exists    boolean;
> BEGIN
>     EXECUTE format(
>         'SELECT EXISTS (SELECT 1 FROM pg_tables WHERE schemaname = $1 AND
> tablename = $2)'
>     ) INTO table_exists USING 'public', table_name;
> 
>     IF NOT table_exists THEN
>         EXECUTE format(
>             'CREATE TABLE IF NOT EXISTS %I PARTITION OF test FOR VALUES FROM
> (%L) TO (%L)'::text,
>             table_name,
>             date_trunc('week', now()),
>             date_trunc('week', now() + interval '1 week')
>         );
>     END IF;
> 
>     RETURN NULL;
> END;
> $$;
> 
> CREATE TRIGGER test_trigger BEFORE INSERT ON test FOR EACH STATEMENT EXECUTE
> PROCEDURE test_trigger_func();
> 
> INSERT INTO test (value) VALUES (1);

Thanks for the report.

The problem here is with the server allowing to create a partition of the
table being inserted into, inside the table's BEFORE INSERT trigger.
Generally speaking, the command that's run inside the trigger shouldn't
have been allowed to proceed if it might change the table's properties
that the execution of the ongoing command is depending upon.

In this case, initialized state of the ongoing command (insert) says that
there is no partition, but the command executed inside the trigger
violated that assumption by adding a partition.  The bug here is that the
command executed by the trigger shouldn't have been allowed to complete.

I propose the attached to fix that.

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/18 20:57, PG Bug reporting form wrote:
>> I tried to use declarative partitioning and, to avoid creating partitions by
>> hand, to make them in ON BEFORE STATEMENT trigger. Trigger executes
>> successfully (proved that with RAISE NOTICE), but server crashes.

> The problem here is with the server allowing to create a partition of the
> table being inserted into, inside the table's BEFORE INSERT trigger.
> Generally speaking, the command that's run inside the trigger shouldn't
> have been allowed to proceed if it might change the table's properties
> that the execution of the ongoing command is depending upon.

Check.

> I propose the attached to fix that.

Hmm ... I wonder if we shouldn't do CheckTableNotInUse for *both* cases,
is_partition or no?

            regards, tom lane

On 2018/10/19 11:52, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> On 2018/10/18 20:57, PG Bug reporting form wrote:
>>> I tried to use declarative partitioning and, to avoid creating partitions by
>>> hand, to make them in ON BEFORE STATEMENT trigger. Trigger executes
>>> successfully (proved that with RAISE NOTICE), but server crashes.
> 
>> The problem here is with the server allowing to create a partition of the
>> table being inserted into, inside the table's BEFORE INSERT trigger.
>> Generally speaking, the command that's run inside the trigger shouldn't
>> have been allowed to proceed if it might change the table's properties
>> that the execution of the ongoing command is depending upon.
> 
> Check.
> 
>> I propose the attached to fix that.
> 
> Hmm ... I wonder if we shouldn't do CheckTableNotInUse for *both* cases,
> is_partition or no?

Yeah, that would be better robustness-wise, but I couldn't think of a case
where not doing CheckTableNotInUse for the !is_partition case would be
problematic.  Adding an inheritance child doesn't change the relcache
content of the parent table, but for partitioning it does.  Maybe I'm
missing something though.

Attached updated patch adds the check for both cases, although I'm not
sure what the error message text added by the patch should look like.  Is
the following OK?

CheckTableNotInUse(relation, "CREATE TABLE INHERITS / PARTITION OF");

Thanks,
Amit

On 2018/10/19 12:45, Amit Langote wrote:
> On 2018/10/19 11:52, Tom Lane wrote:
>> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>>> On 2018/10/18 20:57, PG Bug reporting form wrote:
>>>> I tried to use declarative partitioning and, to avoid creating partitions by
>>>> hand, to make them in ON BEFORE STATEMENT trigger. Trigger executes
>>>> successfully (proved that with RAISE NOTICE), but server crashes.
>>
>>> The problem here is with the server allowing to create a partition of the
>>> table being inserted into, inside the table's BEFORE INSERT trigger.
>>> Generally speaking, the command that's run inside the trigger shouldn't
>>> have been allowed to proceed if it might change the table's properties
>>> that the execution of the ongoing command is depending upon.
>>
>> Check.
>>
>>> I propose the attached to fix that.
>>
>> Hmm ... I wonder if we shouldn't do CheckTableNotInUse for *both* cases,
>> is_partition or no?
> 
> Yeah, that would be better robustness-wise, but I couldn't think of a case
> where not doing CheckTableNotInUse for the !is_partition case would be
> problematic.  Adding an inheritance child doesn't change the relcache
> content of the parent table, but for partitioning it does.  Maybe I'm
> missing something though.
> 
> Attached updated patch adds the check for both cases, although I'm not
> sure what the error message text added by the patch should look like.  Is
> the following OK?
> 
> CheckTableNotInUse(relation, "CREATE TABLE INHERITS / PARTITION OF");

Added this patch to the upcoming CF:

https://commitfest.postgresql.org/20/1836/

Thanks,
Amit

On Fri, Oct 19, 2018 at 12:38:09PM +0300, Dmitry Shalashov wrote:
> That may be off-topic here, but I feel that *some* way of auto-creating
> partitions would be useful though.  Maybe all partitioning setup could
> boil down to one table declaration, with auto-created partitions of
> the same structure, including indices and stuff.

Some patches have been proposed for this purpose during v11 development
timeline if I recall correctly, but they did not cross the finish line.
You could also look at something like pg_partman:
https://github.com/pgpartman/pg_partman
--
Michael

On Fri, Oct 19, 2018 at 12:45:33PM +0900, Amit Langote wrote:
> On 2018/10/19 11:52, Tom Lane wrote:
>> Hmm ... I wonder if we shouldn't do CheckTableNotInUse for *both* cases,
>> is_partition or no?
>
> Yeah, that would be better robustness-wise, but I couldn't think of a case
> where not doing CheckTableNotInUse for the !is_partition case would be
> problematic.  Adding an inheritance child doesn't change the relcache
> content of the parent table, but for partitioning it does.  Maybe I'm
> missing something though.

I am pretty sure that this patch would make some users unhappy.  You
break cases where one may want to add automatically inherited tables.
Here is one example which works normally, but not with the patch
(imagine that the name of the relation is based on some time-related
data, like beginning filling a table for a new month or such):
create table aa (a int);
create function add_inh_func () returns trigger language plpgsql as $$
  begin
  execute 'create table aa2 (a int) inherits (aa)';
  return NULL;
end $$;
create trigger add_inh_trig before insert on aa
  for each statement execute procedure add_inh_func();
  insert into aa values (1);
drop table aa cascade;
drop function add_inh_func ();

In the case of a partition, the execution state relies on the data
inserted, so the restriction sounds fine to me.  If one tries to attach
a partition after creating a table you actually get an error:
ERROR:  55006: cannot ALTER TABLE "check_not_in_use" because it is being
used by active queries in this session
CONTEXT:  SQL statement "alter table check_not_in_use attach partition
check_not_in_use1 for values IN (1);"

You can just trigger that scenario by executing the following queries:
create table check_not_in_use (a int) partition by list (a);
create function add_part_func () returns trigger language plpgsql as $$
  begin
    execute 'create table check_not_in_use1 (a int)';
    execute 'alter table check_not_in_use attach partition
    check_not_in_use1 for values IN (1);';
    return null;
  end $$;
create trigger add_part_trig before insert on
  check_not_in_use
  for each statement execute procedure add_part_func();
insert into check_not_in_use values (1);

> Attached updated patch adds the check for both cases, although I'm not
> sure what the error message text added by the patch should look like.  Is
> the following OK?
>
> CheckTableNotInUse(relation, "CREATE TABLE INHERITS / PARTITION OF");

Other callers of CheckTableNotInUse() use command tags, which is
inconsistent here, still I cannot think of anything better than
CREATE TABLE "%s" PARTITION OF
or:
CREATE TABLE .. PARTITION OF
--
Michael

On 2018/10/29 11:44, Michael Paquier wrote:
> On Fri, Oct 19, 2018 at 12:38:09PM +0300, Dmitry Shalashov wrote:
>> That may be off-topic here, but I feel that *some* way of auto-creating
>> partitions would be useful though.  Maybe all partitioning setup could
>> boil down to one table declaration, with auto-created partitions of
>> the same structure, including indices and stuff.
> 
> Some patches have been proposed for this purpose during v11 development
> timeline if I recall correctly, but they did not cross the finish line.

Hmm, I don't recall any, but maybe I missed some thread.

> You could also look at something like pg_partman:
> https://github.com/pgpartman/pg_partman

I haven't looked at how pg_partman does it, but I suspect it's not doing
it inside a trigger or we would've heard of this crash from pg_partman's
developers. :)  I don't hear you saying it does, but just thought I'd
mention it.

Thanks,
Amit

Hi,

On 2018/10/29 12:31, Michael Paquier wrote:
> On Fri, Oct 19, 2018 at 12:45:33PM +0900, Amit Langote wrote:
>> On 2018/10/19 11:52, Tom Lane wrote:
>>> Hmm ... I wonder if we shouldn't do CheckTableNotInUse for *both* cases,
>>> is_partition or no?
>>
>> Yeah, that would be better robustness-wise, but I couldn't think of a case
>> where not doing CheckTableNotInUse for the !is_partition case would be
>> problematic.  Adding an inheritance child doesn't change the relcache
>> content of the parent table, but for partitioning it does.  Maybe I'm
>> missing something though.
> 
> I am pretty sure that this patch would make some users unhappy.  You
> break cases where one may want to add automatically inherited tables.
> Here is one example which works normally, but not with the patch
> (imagine that the name of the relation is based on some time-related
> data, like beginning filling a table for a new month or such):
> create table aa (a int);
> create function add_inh_func () returns trigger language plpgsql as $$
>   begin
>   execute 'create table aa2 (a int) inherits (aa)';
>   return NULL;
> end $$;
> create trigger add_inh_trig before insert on aa
>   for each statement execute procedure add_inh_func();
>   insert into aa values (1);
> drop table aa cascade;
> drop function add_inh_func ();

Hmm, I can see the point.  As I said, there is no strong reason to include
the inheritance parents in this check afaict, so maybe we could leave that
case out.

> In the case of a partition, the execution state relies on the data
> inserted, so the restriction sounds fine to me.  If one tries to attach
> a partition after creating a table you actually get an error:
> ERROR:  55006: cannot ALTER TABLE "check_not_in_use" because it is being
> used by active queries in this session
> CONTEXT:  SQL statement "alter table check_not_in_use attach partition
> check_not_in_use1 for values IN (1);"
> 
> You can just trigger that scenario by executing the following queries:
> create table check_not_in_use (a int) partition by list (a);
> create function add_part_func () returns trigger language plpgsql as $$
>   begin
>     execute 'create table check_not_in_use1 (a int)';
>     execute 'alter table check_not_in_use attach partition
>     check_not_in_use1 for values IN (1);';
>     return null;
>   end $$;
> create trigger add_part_trig before insert on
>   check_not_in_use
>   for each statement execute procedure add_part_func();
> insert into check_not_in_use values (1);

Yeah, I forgot to include this example in my previous emails.

>> Attached updated patch adds the check for both cases, although I'm not
>> sure what the error message text added by the patch should look like.  Is
>> the following OK?
>>
>> CheckTableNotInUse(relation, "CREATE TABLE INHERITS / PARTITION OF");
> 
> Other callers of CheckTableNotInUse() use command tags, which is
> inconsistent here, still I cannot think of anything better than
> CREATE TABLE "%s" PARTITION OF
> or:
> CREATE TABLE .. PARTITION OF

Maybe the latter is fine, because we want to highlight the parent table's
name, not partition's.

Thanks,
Amit

On Mon, Oct 29, 2018 at 02:28:53PM +0900, Amit Langote wrote:
> On 2018/10/29 12:31, Michael Paquier wrote:
>> In the case of a partition, the execution state relies on the data
>> inserted, so the restriction sounds fine to me.  If one tries to attach
>> a partition after creating a table you actually get an error:
>> ERROR:  55006: cannot ALTER TABLE "check_not_in_use" because it is being
>> used by active queries in this session
>> CONTEXT:  SQL statement "alter table check_not_in_use attach partition
>> check_not_in_use1 for values IN (1);"
>
> Yeah, I forgot to include this example in my previous emails.

We may want to add a new test for this one in the same spirit as what
has been sent in the previous patch.

>> Other callers of CheckTableNotInUse() use command tags, which is
>> inconsistent here, still I cannot think of anything better than
>> CREATE TABLE "%s" PARTITION OF
>> or:
>> CREATE TABLE .. PARTITION OF
>
> Maybe the latter is fine, because we want to highlight the parent table's
> name, not partition's.

Fine by me.
--
Michael

On 2018/10/29 15:57, Michael Paquier wrote:
> On Mon, Oct 29, 2018 at 02:28:53PM +0900, Amit Langote wrote:
>> On 2018/10/29 12:31, Michael Paquier wrote:
>>> In the case of a partition, the execution state relies on the data
>>> inserted, so the restriction sounds fine to me.  If one tries to attach
>>> a partition after creating a table you actually get an error:
>>> ERROR:  55006: cannot ALTER TABLE "check_not_in_use" because it is being
>>> used by active queries in this session
>>> CONTEXT:  SQL statement "alter table check_not_in_use attach partition
>>> check_not_in_use1 for values IN (1);"
>>
>> Yeah, I forgot to include this example in my previous emails.
> 
> We may want to add a new test for this one in the same spirit as what
> has been sent in the previous patch.

OK, added.

>>> Other callers of CheckTableNotInUse() use command tags, which is
>>> inconsistent here, still I cannot think of anything better than
>>> CREATE TABLE "%s" PARTITION OF
>>> or:
>>> CREATE TABLE .. PARTITION OF
>>
>> Maybe the latter is fine, because we want to highlight the parent table's
>> name, not partition's.
> 
> Fine by me.

Done that way.  I also switched the check back to just partitioned parent
case.

Thanks for looking at this.

Regards,
Amit

On Mon, Oct 29, 2018 at 04:32:03PM +0900, Amit Langote wrote:
> Done that way.  I also switched the check back to just partitioned parent
> case.

That seems to be what we are looking for.  Let's wait a couple of days
and see if anybody else has any input to offer on the matter.  I also
would like to think about it again once.
--
Michael

On Tue, Oct 30, 2018 at 01:11:19PM +0900, Michael Paquier wrote:
> That seems to be what we are looking for.  Let's wait a couple of days
> and see if anybody else has any input to offer on the matter.  I also
> would like to think about it again once.

And so I did.

s/paritioned/partitioned/ in the new comment of tablecmds.c.

The tests could be designed better.  We had better not use the same
object names across multiple tests.  If for a reason or another both
test suites are moved to the same series when running in parallel, this
can lead to race conditions which would be annoying to debug.  Test
cases are not much portable as they rely on default partitions and this
bug happens down to v10, where we need to back-patch.  It can be
changed so as we use FOR VALUES (1) or such in the DDL part of the
function.

Those are easy enough to fix and improve, and the patch looks good to
me.  Any objections to commit and back-patch before the next point
release?
--
Michael

Hi,

On 2018/11/02 8:58, Michael Paquier wrote:
> On Tue, Oct 30, 2018 at 01:11:19PM +0900, Michael Paquier wrote:
>> That seems to be what we are looking for.  Let's wait a couple of days
>> and see if anybody else has any input to offer on the matter.  I also
>> would like to think about it again once.
> 
> And so I did.
> 
> s/paritioned/partitioned/ in the new comment of tablecmds.c.
> 
> The tests could be designed better.  We had better not use the same
> object names across multiple tests.  If for a reason or another both
> test suites are moved to the same series when running in parallel, this
> can lead to race conditions which would be annoying to debug.  Test
> cases are not much portable as they rely on default partitions and this
> bug happens down to v10, where we need to back-patch.  It can be
> changed so as we use FOR VALUES (1) or such in the DDL part of the
> function.
> 
> Those are easy enough to fix and improve, and the patch looks good to
> me.

Did you already make those changes yourself and waiting to commit or do
you want me to update the patch?

Thanks,
Amit

On Fri, Nov 02, 2018 at 10:54:41AM +0900, Amit Langote wrote:
> Did you already make those changes yourself and waiting to commit or do
> you want me to update the patch?

No need to send a new patch, thanks for caring!
--
Michael

On 2018/11/02 11:30, Michael Paquier wrote:
> On Fri, Nov 02, 2018 at 10:54:41AM +0900, Amit Langote wrote:
>> Did you already make those changes yourself and waiting to commit or do
>> you want me to update the patch?
> 
> No need to send a new patch, thanks for caring!

Ah, okay.  Thanks for taking care of that!

Regards,
Amit

On Fri, Nov 02, 2018 at 11:45:29AM +0900, Amit Langote wrote:
> On 2018/11/02 11:30, Michael Paquier wrote:
>> On Fri, Nov 02, 2018 at 10:54:41AM +0900, Amit Langote wrote:
>>> Did you already make those changes yourself and waiting to commit or do
>>> you want me to update the patch?
>>
>> No need to send a new patch, thanks for caring!
>
> Ah, okay.  Thanks for taking care of that!

And committed after fixing the typo, improving the test portability for
10, and switching the object names in the regression tests.
--
Michael

On 2018/11/05 11:09, Michael Paquier wrote:
> On Fri, Nov 02, 2018 at 11:45:29AM +0900, Amit Langote wrote:
>> On 2018/11/02 11:30, Michael Paquier wrote:
>>> On Fri, Nov 02, 2018 at 10:54:41AM +0900, Amit Langote wrote:
>>>> Did you already make those changes yourself and waiting to commit or do
>>>> you want me to update the patch?
>>>
>>> No need to send a new patch, thanks for caring!
>>
>> Ah, okay.  Thanks for taking care of that!
> 
> And committed after fixing the typo, improving the test portability for
> 10, and switching the object names in the regression tests.

Thank you for committing.

Regards,
Amit

On 2018-Oct-29, Michael Paquier wrote:

> I am pretty sure that this patch would make some users unhappy.  You
> break cases where one may want to add automatically inherited tables.

I think this problem would be fixed by my patch here:
https://postgr.es/m/20181025202622.d3x4y4ch7m4pxwnn@alvherre.pgsql

I guess I'm just saying that unhappy users could help review that patch
;-)

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

On Mon, Nov 05, 2018 at 04:23:05PM -0300, Alvaro Herrera wrote:
> I think this problem would be fixed by my patch here:
> https://postgr.es/m/20181025202622.d3x4y4ch7m4pxwnn@alvherre.pgsql
>
> I guess I'm just saying that unhappy users could help review that patch
> ;-)

If that's the case, it may be adapted to rebase the patch on top of
dc3e436b, then remove the limitation and rework the tests' comments.
Removing this limitation would be really nice.
--
Michael