pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Fix table rewrites that include a column without a default.
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2019-10-16 ae5cae54ca 3b25de6209 Replace alter_table.sql test usage of event triggers.
* pgsql: Fix table rewrites that include a column without a default.
@ 2019-10-10 05:19 Andres Freund <andres@anarazel.de>
2019-10-10 20:51 ` Re: pgsql: Fix table rewrites that include a column without a default. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2019-10-10 05:19 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Fix table rewrites that include a column without a default.
In c2fe139c201c I made ATRewriteTable() use tuple slots. Unfortunately
I did not notice that columns can be added in a rewrite that do not
have a default, when another column is added/altered requiring one.
Initialize columns to NULL again, and add tests.
Bug: #16038
Reported-By: anonymous
Author: Andres Freund
Discussion: https://postgr.es/m/16038-5c974541f2bf6749@postgresql.org
Backpatch: 12, where the bug was introduced in c2fe139c201c
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/93765bd956bea26206043de8cbb0ae4b67e4df15
Modified Files
--------------
src/backend/commands/tablecmds.c | 10 +++++++
src/test/regress/expected/alter_table.out | 47 ++++++++++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 48 +++++++++++++++++++++++++++++++
3 files changed, 105 insertions(+)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Fix table rewrites that include a column without a default.
2019-10-10 05:19 pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
@ 2019-10-10 20:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-10 20:59 ` Re: pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2019-10-10 20:51 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-committers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> Fix table rewrites that include a column without a default.
This patch added use of an event trigger in alter_table.sql.
As we have learned the hard way, it's not acceptable to create event
triggers in test scripts that run in parallel with anything else,
because they will intermittently screw up DDL happening in the
parallel scripts. As seen just now on clam:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=clam&dt=2019-10-10%2020%3A16%3A19
inline patch 1: /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/results/plpgsql.out (+1/-0 lines) download inline-1.patch
diff -U3 /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/expected/plpgsql.out /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/results/plpgsql.out
--- /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/expected/plpgsql.out 2019-05-09 09:00:10.653795677 +0100
+++ /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/results/plpgsql.out 2019-10-10 21:21:46.406007734 +0100
@@ -5419,6 +5419,7 @@
-- 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
* Re: pgsql: Fix table rewrites that include a column without a default.
2019-10-10 05:19 pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
2019-10-10 20:51 ` Re: pgsql: Fix table rewrites that include a column without a default. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-10-10 20:59 ` Andres Freund <andres@anarazel.de>
2019-10-10 21:16 ` Re: pgsql: Fix table rewrites that include a column without a default. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2019-10-10 20:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
Hi,
On 2019-10-10 16:51:23 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Fix table rewrites that include a column without a default.
>
> This patch added use of an event trigger in alter_table.sql.
> As we have learned the hard way, it's not acceptable to create event
> triggers in test scripts that run in parallel with anything else,
> because they will intermittently screw up DDL happening in the
> parallel scripts. As seen just now on clam:
Dang :(
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=clam&dt=2019-10-10%2020%3A16%3A19
>
> diff -U3 /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/expected/plpgsql.out /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/results/plpgsql.out
> --- /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/expected/plpgsql.out 2019-05-09 09:00:10.653795677 +0100
> +++ /home/pgbfarm/buildroot/HEAD/pgsql.build/src/test/regress/results/plpgsql.out 2019-10-10 21:21:46.406007734 +0100
> @@ -5419,6 +5419,7 @@
> -- 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
>
>
> Do you *really* need an event trigger to test this?
No. I added it to avoid the tests getting "accidentally" optimized away,
the next time somebody adds some ALTER TABLE optimizations. The fast
default stuff had removed coverage from a test that'd have uncovered
this bug. And an event trigger seemed to be the easiest way to achieve
that.
The only real alternative I can see to just removing the trigger is
embedding a check in the trigger that will cause it to only work in the
current session. Something like SET regress.log_table_rewrites, and a
IF current_setting('regress.log_table_rewrites')::bool in the function.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Fix table rewrites that include a column without a default.
2019-10-10 05:19 pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
2019-10-10 20:51 ` Re: pgsql: Fix table rewrites that include a column without a default. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-10 20:59 ` Re: pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
@ 2019-10-10 21:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-16 05:58 ` Re: pgsql: Fix table rewrites that include a column without a default. Michael Paquier <michael@paquier.xyz>
2019-10-16 09:48 ` Re: pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 7+ messages in thread
From: Tom Lane @ 2019-10-10 21:16 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-committers@lists.postgresql.org
Andres Freund <andres@anarazel.de> writes:
> On 2019-10-10 16:51:23 -0400, Tom Lane wrote:
>> Do you *really* need an event trigger to test this?
> No. I added it to avoid the tests getting "accidentally" optimized away,
> the next time somebody adds some ALTER TABLE optimizations. The fast
> default stuff had removed coverage from a test that'd have uncovered
> this bug. And an event trigger seemed to be the easiest way to achieve
> that.
> The only real alternative I can see to just removing the trigger is
> embedding a check in the trigger that will cause it to only work in the
> current session. Something like SET regress.log_table_rewrites, and a
> IF current_setting('regress.log_table_rewrites')::bool in the function.
If you just want to verify that a rewrite happened or didn't happen,
seems like you could check whether the table's relfilenode changed.
regression=# select relfilenode as oldfilenode from pg_class where relname = 'rewrite_test'
regression-# \gset
regression=# select relfilenode != :oldfilenode as changed from pg_class where relname = 'rewrite_test';
changed
---------
f
(1 row)
regression=# alter table rewrite_test alter column notempty3_norewrite type bigint;
ALTER TABLE
regression=# select relfilenode != :oldfilenode as changed from pg_class where relname = 'rewrite_test';
changed
---------
t
(1 row)
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.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Fix table rewrites that include a column without a default.
2019-10-10 05:19 pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
2019-10-10 20:51 ` Re: pgsql: Fix table rewrites that include a column without a default. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-10 20:59 ` Re: pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
2019-10-10 21:16 ` Re: pgsql: Fix table rewrites that include a column without a default. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-10-16 05:58 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 7+ messages in thread
From: Michael Paquier @ 2019-10-16 05:58 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-committers@lists.postgresql.org
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) download
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/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 = '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.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;
-- 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;
--
* Re: pgsql: Fix table rewrites that include a column without a default.
2019-10-10 05:19 pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
2019-10-10 20:51 ` Re: pgsql: Fix table rewrites that include a column without a default. Tom Lane <tgl@sss.pgh.pa.us>
2019-10-10 20:59 ` Re: pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
2019-10-10 21:16 ` Re: pgsql: Fix table rewrites that include a column without a default. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-10-16 09:48 ` Andres Freund <andres@anarazel.de>
1 sibling, 0 replies; 7+ messages in thread
From: Andres Freund @ 2019-10-16 09:48 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
Hi,
On 2019-10-10 17:16:08 -0400, Tom Lane wrote:
> If you just want to verify that a rewrite happened or didn't happen,
> seems like you could check whether the table's relfilenode changed.
>
> regression=# select relfilenode as oldfilenode from pg_class where relname = 'rewrite_test'
> regression-# \gset
> regression=# select relfilenode != :oldfilenode as changed from pg_class where relname = 'rewrite_test';
> changed
> ---------
> f
> (1 row)
> regression=# alter table rewrite_test alter column notempty3_norewrite type bigint;
> ALTER TABLE
> regression=# select relfilenode != :oldfilenode as changed from pg_class where relname = 'rewrite_test';
> changed
> ---------
> t
> (1 row)
This felt verbose to me over a number of to-be-tested statements, so I
instead replaced it with a plpgsql function that EXECUTEs the DDL and
returns whether a rewrite happened. I'll assume/hope that that fixes the
occasional bf failures.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Fix table rewrites that include a column without a default.
@ 2019-10-10 05:19 Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 7+ messages in thread
From: Andres Freund @ 2019-10-10 05:19 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Fix table rewrites that include a column without a default.
In c2fe139c201c I made ATRewriteTable() use tuple slots. Unfortunately
I did not notice that columns can be added in a rewrite that do not
have a default, when another column is added/altered requiring one.
Initialize columns to NULL again, and add tests.
Bug: #16038
Reported-By: anonymous
Author: Andres Freund
Discussion: https://postgr.es/m/16038-5c974541f2bf6749@postgresql.org
Backpatch: 12, where the bug was introduced in c2fe139c201c
Branch
------
REL_12_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/f224c7c11ea7be2751e3342e11317070ffb5622d
Modified Files
--------------
src/backend/commands/tablecmds.c | 10 +++++++
src/test/regress/expected/alter_table.out | 47 ++++++++++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 48 +++++++++++++++++++++++++++++++
3 files changed, 105 insertions(+)
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2019-10-16 09:48 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10 05:19 pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
2019-10-10 20:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-10 20:59 ` Andres Freund <andres@anarazel.de>
2019-10-10 21:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-16 05:58 ` Michael Paquier <michael@paquier.xyz>
2019-10-16 09:48 ` Andres Freund <andres@anarazel.de>
2019-10-10 05:19 pgsql: Fix table rewrites that include a column without a default. Andres Freund <andres@anarazel.de>
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