Date: Mon, 29 Oct 2018 12:31:17 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, skaurus@gmail.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15437: Segfault during insert into declarative partitioned
 table with a trigger creating partition
Message-ID: <20181029033117.GE14242@paquier.xyz>
References: <15437-3fe01ee66bd1bae1@postgresql.org>
 <e8079b1f-3e41-d1dc-1575-ff3cce7d76ac@lab.ntt.co.jp>
 <8213.1539917571@sss.pgh.pa.us>
 <ddc70376-9ddb-8637-eb69-aa7ae6f13378@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ey/N+yb7u/X9mFhi"
Content-Disposition: inline
In-Reply-To: <ddc70376-9ddb-8637-eb69-aa7ae6f13378@lab.ntt.co.jp>
Precedence: bulk


--ey/N+yb7u/X9mFhi
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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?
>=20
> 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?
>=20
> 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

--ey/N+yb7u/X9mFhi
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlvWfwUACgkQnvQgOdby
QH19tQ//UCSb5lD7OWAWNn708UzlNQo06gPf/sWBzOiQ066Q5o0G1wtCNIc5piky
IRQV35bK3YTjc01VzPi/nHUX88eqoL5nOh+oERpVB5nwrHYtjdX4YTeLpYUQntZz
yCREPBCVO/MMBDWImMk4IwOAoHgkNBKFp82XaLLjAle620ADHZOrsGg5m4041/5x
D5X+/J8UrJM8bo940neXn1cm6HhaT8tVmZksGB8J/ux/zqRpT9PeJefwSaM68LKC
sITjhBK/9CGCdl9TnOmuAtWFBX7BLub2A+/AimKUIVwD5JYPRMan+TfIIhi0V6dR
VCdAagBgMcGJzpURpaRWOiV7O++UwbyT07LCQuKroBlKQsk7ZUQccVJ4yVD5ij5B
IzFjJFJVfQnuAHhBYl2zybHtxFla72pui5698UUCWlHvDyE/gZhkk0ZXyMrGd2wG
JcEcE3f81u3m0dRAlN8WGF50gkROOpg90/E6MUwvTgwpZIAh1lUm6KCjydcXLTtw
2gDc9VWecz0+QuKVpvZvUhYKB1Q1yUu079CnL82Vt6FYzmTSrpsKn30zDFN8zh2e
9WjgJn0tmkV7VwWdCAVjQG/E9TIqnM2H64xenJTHKbMUyTwAaDsk0DcDuZ5TFebA
CKeJGgonb8aBbk7nWIHEJO5xks8uoZ4d2FpVTYOEhRDP9SXxaWY=
=Xb9t
-----END PGP SIGNATURE-----

--ey/N+yb7u/X9mFhi--