Date: Wed, 16 Oct 2019 14:58:17 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>,
	pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Fix table rewrites that include a column without a
 default.
Message-ID: <20191016055817.GE2602@paquier.xyz>
References: <E1iIQrV-0005gk-1D@gemulon.postgresql.org>
 <3328.1570740683@sss.pgh.pa.us>
 <20191010205903.tj73q3qcyfft3izz@alap3.anarazel.de>
 <4541.1570742168@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="XStn23h1fwudRqtG"
Content-Disposition: inline
In-Reply-To: <4541.1570742168@sss.pgh.pa.us>
Precedence: bulk


--XStn23h1fwudRqtG
Content-Type: multipart/mixed; boundary="zaRBsRFn0XYhEU69"
Content-Disposition: inline


--zaRBsRFn0XYhEU69
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Oct 10, 2019 at 05:16:08PM -0400, Tom Lane wrote:
> I really really don't want an event trigger running in everything that
> runs in parallel with alter_table.  That's a debugging nightmare of
> monster proportions.

+1, that's not stable.  elver has just complained about a side effect
of this commit:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=elver&dt=2019-10-16%2005%3A04%3A24
 -- now change 'name' to an integer to see what happens...
 ALTER TABLE alter_table_under_transition_tables
 ALTER COLUMN name TYPE int USING name::integer;
+WARNING:  rewriting table alter_table_under_transition_tables
 UPDATE alter_table_under_transition_tables
 SET name = (name::text || name::text)::integer;
 WARNING:  old table = 1=11,2=22,3=33, new table = 1=1111,2=2222,3=3333

Another method would be to simply store the past relfilenodes into a
temporary table with the relation OID, and do a comparison with what's
in pg_class post-rewrite as some partition-related tests do in the
same file.  For one relation the \gset method is more readable
anyway.

If you really wish to keep an event trigger, you could also apply a
filter within the function to only consider rewrite_test as something
to report about, still I would recommend avoiding global effects if
not necessary.

So I would fix the issue as per the attached, as Tom has suggested.
Thoughts?
--
Michael

--zaRBsRFn0XYhEU69
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="alter-table-tests.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/e=
xpected/alter_table.out
index 41a7bcd6d0..b041247476 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2437,51 +2437,96 @@ drop view at_view_2;
 drop view at_view_1;
 drop table at_base_table;
 -- check adding a column not iself requiring a rewrite, together with
--- a column requiring a default (bug #16038)
--- ensure that rewrites aren't silently optimized away, removing the
--- value of the test
-CREATE OR REPLACE FUNCTION evtrig_rewrite_log() RETURNS event_trigger
-LANGUAGE plpgsql AS $$
-BEGIN
-     RAISE WARNING 'rewriting table %',
-        pg_event_trigger_table_rewrite_oid()::regclass;
-END;
-$$;
-CREATE EVENT TRIGGER evtrig_rewrite_log ON table_rewrite
-    EXECUTE PROCEDURE evtrig_rewrite_log();
+-- a column requiring a default (bug #16038).  This checks
+-- pg_class.relfilenode before and after the rewrite for differences
+-- to see if a rewrite has been done or not.
 CREATE TABLE rewrite_test(col text);
 INSERT INTO rewrite_test VALUES ('something');
 INSERT INTO rewrite_test VALUES (NULL);
 -- empty[12] doesn't need rewrite, but notempty[12]_rewrite will force one
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN empty1 text,
     ADD COLUMN notempty1_rewrite serial;
-WARNING:  rewriting table rewrite_test
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+ changed=20
+---------
+ t
+(1 row)
+
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN notempty2_rewrite serial,
     ADD COLUMN empty2 text;
-WARNING:  rewriting table rewrite_test
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+ changed=20
+---------
+ t
+(1 row)
+
 -- also check that fast defaults cause no problem, first without rewrite
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN empty3 text,
     ADD COLUMN notempty3_norewrite int default 42;
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+ changed=20
+---------
+ f
+(1 row)
+
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN notempty4_norewrite int default 42,
     ADD COLUMN empty4 text;
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+ changed=20
+---------
+ f
+(1 row)
+
 -- then with rewrite
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN empty5 text,
     ADD COLUMN notempty5_norewrite int default 42,
     ADD COLUMN notempty5_rewrite serial;
-WARNING:  rewriting table rewrite_test
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+ changed=20
+---------
+ t
+(1 row)
+
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN notempty6_rewrite serial,
     ADD COLUMN empty6 text,
     ADD COLUMN notempty6_norewrite int default 42;
-WARNING:  rewriting table rewrite_test
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+ changed=20
+---------
+ t
+(1 row)
+
 -- cleanup
-drop event trigger evtrig_rewrite_log;
-drop function evtrig_rewrite_log();
 DROP TABLE rewrite_test;
 --
 -- lock levels
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/al=
ter_table.sql
index 3fde96b2b9..67672008ff 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1551,51 +1551,71 @@ drop view at_view_1;
 drop table at_base_table;
=20
 -- check adding a column not iself requiring a rewrite, together with
--- a column requiring a default (bug #16038)
-
--- ensure that rewrites aren't silently optimized away, removing the
--- value of the test
-CREATE OR REPLACE FUNCTION evtrig_rewrite_log() RETURNS event_trigger
-LANGUAGE plpgsql AS $$
-BEGIN
-     RAISE WARNING 'rewriting table %',
-        pg_event_trigger_table_rewrite_oid()::regclass;
-END;
-$$;
-CREATE EVENT TRIGGER evtrig_rewrite_log ON table_rewrite
-    EXECUTE PROCEDURE evtrig_rewrite_log();
-
+-- a column requiring a default (bug #16038).  This checks
+-- pg_class.relfilenode before and after the rewrite for differences
+-- to see if a rewrite has been done or not.
 CREATE TABLE rewrite_test(col text);
 INSERT INTO rewrite_test VALUES ('something');
 INSERT INTO rewrite_test VALUES (NULL);
=20
 -- empty[12] doesn't need rewrite, but notempty[12]_rewrite will force one
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN empty1 text,
     ADD COLUMN notempty1_rewrite serial;
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN notempty2_rewrite serial,
     ADD COLUMN empty2 text;
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+
 -- also check that fast defaults cause no problem, first without rewrite
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN empty3 text,
     ADD COLUMN notempty3_norewrite int default 42;
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN notempty4_norewrite int default 42,
     ADD COLUMN empty4 text;
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+
 -- then with rewrite
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN empty5 text,
     ADD COLUMN notempty5_norewrite int default 42,
     ADD COLUMN notempty5_rewrite serial;
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
+SELECT relfilenode AS oldfilenode FROM pg_class
+    WHERE relname =3D 'rewrite_test'
+    \gset
 ALTER TABLE rewrite_test
     ADD COLUMN notempty6_rewrite serial,
     ADD COLUMN empty6 text,
     ADD COLUMN notempty6_norewrite int default 42;
+SELECT relfilenode !=3D :oldfilenode AS changed FROM pg_class
+    WHERE relname =3D 'rewrite_test';
=20
 -- cleanup
-drop event trigger evtrig_rewrite_log;
-drop function evtrig_rewrite_log();
 DROP TABLE rewrite_test;
=20
 --

--zaRBsRFn0XYhEU69--

--XStn23h1fwudRqtG
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl2msXkACgkQnvQgOdby
QH05vQ/+JPw439Dd90Maik+LBAxGKfiUvLPA8MOTLVi3TiBpBPKrBPdhb9Gd0ucp
zFMZ8RuKGHSAjqcgbFTufL7ieqHhcxB0+yUNY75A8yrqwRRiuoIA27RSSs3Wj8sL
dPlvW0wf0hEET4npyo6N3POX+m1tYjtm+wsc6UixWc4f1ywDlwXFzfV0V15yKm3U
RjHkq0qWOKJcu6Beycm4upCNDY+oy5dTC5KWZbWYFW6ieos7ETutNBd9Hep9tGKc
DfqdwBCNPWYafu59T7ppxF4OGMZRIdfwga2rvq/2lcJSTtKuyeSo6D+gWVtqlmEf
CeOW4Dt+nT/XQQTR7LBmOdz5YUCjgDkSIAP/A9BCOAJDKG9OWHeb34kNxTOHxUFL
thI9rWQ++F53XHkWhjvNpQH7S5bRuQ82Vjj3yuO+OEv2ge+2Oddir+hUcJi5VECo
TGuUILwiN0ibKm3F9oeP36vpNQODDO4M9TkMJBb+I9CpoWpdUt+VXh9jNgNJSjWX
l3GIBUIq3tgt+ntkpcyapGBF8ymtxiybhuM57lW2KqQwfZgdaFcNltXwllPX7E+t
pDf4eMBZZrGfJbAP+sI1/KknuYkD/aHwv/KiMJX8y1iJbfoyunB0NBO8D0HPwR/U
9rzerv2fIdqHRKzvCfek8IV1eccY3ywkHCkvadkp2oPiipKfht8=
=KFrc
-----END PGP SIGNATURE-----

--XStn23h1fwudRqtG--