Re: BUG #15437: Segfault during insert into declarative partitionedtable with a trigger creating partition

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