Subject: Re: using index or check in ALTER TABLE SET NOT NULL
To: Tom Lane <tgl@sss.pgh.pa.us>, Sergei Kornilov <sk@zsrv.org>
References: <880511520351578@web5j.yandex.ru>
 <20180306172528.xnelm3xcq2qeyd2g@alvherre.pgsql>
 <1715451520361128@web26o.yandex.ru> <1142.1520362313@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Stephen Frost <sfrost@snowman.net>, Robert Haas <robertmhaas@gmail.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
From: Ildar Musin <i.musin@postgrespro.ru>
Message-ID: <082cf493-a989-dcd6-7baf-a94a40d07ea0@postgrespro.ru>
Date: Wed, 7 Mar 2018 14:03:53 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <1142.1520362313@sss.pgh.pa.us>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Sergei, Alvaro, Tom,

On 06.03.2018 20:25, Alvaro Herrera wrote:
> You should be able to use an event trigger that raises a message when
> table_rewrite is hit, to notify the test driver that a rewrite happens.
>
> (If any DDL that causes a table rewrite fails to trigger the
> table_rewrite event correctly, that is a bug.)
>

It seems that 'table_rewrite' event trigger isn't fired in case of 
simple scan (without actual rewrite).

> ISTM that depending on DEBUG messages is bad because debugging lines
> added elsewhere will make your tests fail;

I think that between test depending on DEBUG message and no test at all 
the former one is preferable. Are there other options to test it?

On 06.03.2018 21:51, Tom Lane wrote:
>
> Do you actually need test output proving that this code path was taken
> rather than the default one?  Seems like looking at the code coverage
> report might be enough.
>

Code coverage cannot guarantee correctness. I think there should be at 
least two tests:
* full scan is performed when there are no check constraints that 
restrict NULL values;
* full scan is omitted when there are such constraints.

The last one could also include some unobvious cases like CHECK (col > 
0), complex expressions with boolean operators etc.


PS: Btw, some check constraints that implies NOT NULL still won't 
prevent from full table scan. For instance:

# create table test (a int, b int check ((a + b) is not null));
CREATE TABLE

# set client_min_messages to 'debug1';
SET

# insert into test values (1, 1);
INSERT 0 1

# alter table test alter column a set not null;
DEBUG:  verifying table "test"   <<<< full table scan!
ALTER TABLE

Since operator+ (aka int4pl) is strict it returns NULL if at least one 
of its operands is NULL. And this check constraint ensures therefore 
that both columns 'a' and 'b' are NOT NULL. It isn't probably the issue 
of this patch, just something that someone may find interesting.

-- 
Ildar Musin
i.musin@postgrespro.ru