Date: Thu, 10 Oct 2019 13:59:03 -0700
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Fix table rewrites that include a column without a
 default.
Message-ID: <20191010205903.tj73q3qcyfft3izz@alap3.anarazel.de>
References: <E1iIQrV-0005gk-1D@gemulon.postgresql.org>
 <3328.1570740683@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3328.1570740683@sss.pgh.pa.us>
Precedence: bulk

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