From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Fix table rewrites that include a column without a default.
Date: Wed, 16 Oct 2019 14:58:17 +0900
Message-ID: <20191016055817.GE2602@paquier.xyz> (raw)
In-Reply-To: <4541.1570742168@sss.pgh.pa.us>
References: <E1iIQrV-0005gk-1D@gemulon.postgresql.org>
<3328.1570740683@sss.pgh.pa.us>
<20191010205903.tj73q3qcyfft3izz@alap3.anarazel.de>
<4541.1570742168@sss.pgh.pa.us>
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
Attachments:alter-table-tests.patch (text/x-diff, 6.8K) downloaddiff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.outindex 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 = 'rewrite_test'+ \gset
ALTER TABLE rewrite_test
ADD COLUMN empty1 text,
ADD COLUMN notempty1_rewrite serial;
-WARNING: rewriting table rewrite_test+SELECT relfilenode != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+ changed +---------+ t+(1 row)++SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = 'rewrite_test'+ \gset
ALTER TABLE rewrite_test
ADD COLUMN notempty2_rewrite serial,
ADD COLUMN empty2 text;
-WARNING: rewriting table rewrite_test+SELECT relfilenode != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+ changed +---------+ t+(1 row)+
-- also check that fast defaults cause no problem, first without rewrite
+SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = 'rewrite_test'+ \gset
ALTER TABLE rewrite_test
ADD COLUMN empty3 text,
ADD COLUMN notempty3_norewrite int default 42;
+SELECT relfilenode != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+ changed +---------+ f+(1 row)++SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = 'rewrite_test'+ \gset
ALTER TABLE rewrite_test
ADD COLUMN notempty4_norewrite int default 42,
ADD COLUMN empty4 text;
+SELECT relfilenode != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+ changed +---------+ f+(1 row)+
-- then with rewrite
+SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = '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 != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+ changed +---------+ t+(1 row)++SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = '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 != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+ changed +---------+ 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/alter_table.sqlindex 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;
-- 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 = 'rewrite_test'+ \gset
ALTER TABLE rewrite_test
ADD COLUMN empty1 text,
ADD COLUMN notempty1_rewrite serial;
+SELECT relfilenode != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';++SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = 'rewrite_test'+ \gset
ALTER TABLE rewrite_test
ADD COLUMN notempty2_rewrite serial,
ADD COLUMN empty2 text;
+SELECT relfilenode != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+
-- also check that fast defaults cause no problem, first without rewrite
+SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = 'rewrite_test'+ \gset
ALTER TABLE rewrite_test
ADD COLUMN empty3 text,
ADD COLUMN notempty3_norewrite int default 42;
+SELECT relfilenode != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = 'rewrite_test'+ \gset
ALTER TABLE rewrite_test
ADD COLUMN notempty4_norewrite int default 42,
ADD COLUMN empty4 text;
+SELECT relfilenode != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+
-- then with rewrite
+SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = '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 != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';+SELECT relfilenode AS oldfilenode FROM pg_class+ WHERE relname = '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 != :oldfilenode AS changed FROM pg_class+ WHERE relname = 'rewrite_test';
-- cleanup
-drop event trigger evtrig_rewrite_log;-drop function evtrig_rewrite_log();
DROP TABLE rewrite_test;
--
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: michael@paquier.xyz, tgl@sss.pgh.pa.us, andres@anarazel.de, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Fix table rewrites that include a column without a default.
In-Reply-To: <20191016055817.GE2602@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox