Date: Wed, 9 Oct 2019 22:21:33 -0700
From: Andres Freund <andres@anarazel.de>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: deathlock13@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16038: Alter table - SegFault
Message-ID: <20191010052133.eqfw5euzbrtf5nwu@alap3.anarazel.de>
References: <16038-5c974541f2bf6749@postgresql.org>
 <20191004144352.GA15934@alvherre.pgsql>
 <20191004152624.6hcdsyqgp6qntw2h@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191004152624.6hcdsyqgp6qntw2h@alap3.anarazel.de>
Precedence: bulk

Hi,

On 2019-10-04 08:26:24 -0700, Andres Freund wrote:
> On 2019-10-04 11:43:52 -0300, Alvaro Herrera wrote:
> > On 2019-Oct-04, PG Bug reporting form wrote:
> > 
> > > alter table test.testa
> > > add column idb numeric(10,0) NOT NULL DEFAULT nextval('test.sq_testb'),
> > > add column fk_tmpb varchar(20);
> > > 
> > > server process (PID 21884) was terminated by signal 11: Segmentation fault
> > > - empty table - alter goes ok , split alter into 2 add col - works too

Thanks for the bug report!  I've pushed a fix, which will be included in
the next minor release. If you need a workaround until then, you can
avoid the crash by only adding one column add a time.


> > Hmm, confirmed, here's the stack trace:
> 
> The trailing attributes in the new slot in AtRewriteTable() aren't
> necessarily set to isnull=true. So to trigger the problem, one needs a
> rewrite triggered by *another* column (otherwise we'll not hit this
> path), combined with a new column that doesn't have a default value.  I
> think that's probably my bug.
> 
> This seems to fix the problem:
> 
> diff --git i/src/backend/commands/tablecmds.c w/src/backend/commands/tablecmds.c
> index 05593f33162..6f72b08a87d 100644
> --- i/src/backend/commands/tablecmds.c
> +++ w/src/backend/commands/tablecmds.c
> @@ -4890,6 +4890,14 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
>                                                 table_slot_callbacks(oldrel));
>              newslot = MakeSingleTupleTableSlot(newTupDesc,
>                                                 table_slot_callbacks(newrel));
> +
> +            /*
> +             * Set all columns in the new slot to NULL initially, to ensure
> +             * columns added as part of the rewrite are initialized to
> +             * NULL. That's necessary as tab->newvals will not contain an
> +             * expression for columns with a NULL default.
> +             */
> +            ExecStoreAllNullTuple(newslot);
>          }

Pushed, after adding some tests.

Greetings,

Andres Freund