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