Обсуждение: pgsql: Fix table rewrites that include a column without a default.
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(+)
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 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? regards, tom lane
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
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
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
Вложения
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