Re: using index or check in ALTER TABLE SET NOT NULL

[Tom Lane]

Tomas Vondra  writes:
> I think there's a consensus to change INFO to DEBUG1 in pg12, and then
> maybe imlpement something like VERBOSE mode in the future. Objections?

ISTM the consensus is "we'd rather reduce the verbosity, but we don't
want to give up test coverage".  So what's blocking this is lack of
a patch to show that there's another way to verify what code path
was taken.

> As for the reduction of test coverage, can't we deduce whether a
> constraint was used from data in pg_stats or something like that?

Not sure how exactly ... and we've already learned that pg_stats
isn't too reliable.

regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

[Tomas Vondra]

On Wed, Jun 12, 2019 at 08:34:57AM +1200, David Rowley wrote:

On Tue, 11 Jun 2019 at 03:35, Sergei Kornilov  wrote:

> Does anyone think we shouldn't change the INFO message in ATTACH
> PARTITION to a DEBUG1 in PG12?

Seems no one wants to vote against this change.


I'm concerned about two things:

1. The patch reduces the test coverage of ATTACH PARTITION. We now
have no way to ensure the constraint was used to validate the rows in
the partition.
2. We're inconsistent with what we do for SET NOT NULL and ATTACH
PARTITION. We raise an INFO message when we use a constraint for
ATTACH PARTITION and only a DEBUG1 for SET NOT NULL.

Unfortunately, my two concerns conflict with each other.



We're getting close to beta3/rc1, and this thread was idle for ~1 month.

I think there's a consensus to change INFO to DEBUG1 in pg12, and then
maybe imlpement something like VERBOSE mode in the future. Objections?

As for the reduction of test coverage, can't we deduce whether a
constraint was used from data in pg_stats or something like that?

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Tue, 11 Jun 2019 at 03:35, Sergei Kornilov  wrote:
> > Does anyone think we shouldn't change the INFO message in ATTACH
> > PARTITION to a DEBUG1 in PG12?
>
> Seems no one wants to vote against this change.

I'm concerned about two things:

1. The patch reduces the test coverage of ATTACH PARTITION. We now
have no way to ensure the constraint was used to validate the rows in
the partition.
2. We're inconsistent with what we do for SET NOT NULL and ATTACH
PARTITION. We raise an INFO message when we use a constraint for
ATTACH PARTITION and only a DEBUG1 for SET NOT NULL.

Unfortunately, my two concerns conflict with each other.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello

> Does anyone think we shouldn't change the INFO message in ATTACH
> PARTITION to a DEBUG1 in PG12?

Seems no one wants to vote against this change.

My proposed patch was here: https://commitfest.postgresql.org/23/2076/

regards, Sergei

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Fri, 3 May 2019 at 19:06, David Rowley  wrote:
> FWIW, I'm in favour of changing ATTACH PARTITION's INFO messages to
> DEBUG1 for PG12 to align it to what bbb96c3704c did.
>
> Anyone else?

Does anyone think we shouldn't change the INFO message in ATTACH
PARTITION to a DEBUG1 in PG12?

I think it makes sense make this change so that it matches the
behaviour of the output of ALTER TABLE .. ALTER COLUMN .. SET NOT NULL
when it uses a CHECK constraint.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Thu, 2 May 2019 at 14:22, David Rowley  wrote:
>
> On Thu, 2 May 2019 at 13:08, Tom Lane  wrote:
> >
> > Not a blocker perhaps, but it's better if we can get new behavior to
> > be more or less right the first time.
>
> It's not really new behaviour though. The code in question is for
> ATTACH PARTITION and was added in c31e9d4bafd (back in 2017).
> bbb96c3704c is the commit for using constraints to speed up SET NOT
> NULL. The mention of using the constraint for proof was made DEBUG1 in
> the initial commit.  What we need to decide is if we want to make
> ATTACH PARTITION's INFO message a DEBUG1 in pg12 or not.

FWIW, I'm in favour of changing ATTACH PARTITION's INFO messages to
DEBUG1 for PG12 to align it to what bbb96c3704c did.

Anyone else?

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Thu, 2 May 2019 at 13:08, Tom Lane  wrote:
>
> David Rowley  writes:
> > On Thu, 2 May 2019 at 06:18, Sergei Kornilov  wrote:
> >> PS: not think this is blocker for v12
>
> > Probably not a blocker, but now does not seem like an unreasonable
> > time to lower these INFO messages down to DEBUG.
>
> Not a blocker perhaps, but it's better if we can get new behavior to
> be more or less right the first time.

It's not really new behaviour though. The code in question is for
ATTACH PARTITION and was added in c31e9d4bafd (back in 2017).
bbb96c3704c is the commit for using constraints to speed up SET NOT
NULL. The mention of using the constraint for proof was made DEBUG1 in
the initial commit.  What we need to decide is if we want to make
ATTACH PARTITION's INFO message a DEBUG1 in pg12 or not.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Tom Lane]

David Rowley  writes:
> On Thu, 2 May 2019 at 06:18, Sergei Kornilov  wrote:
>> PS: not think this is blocker for v12

> Probably not a blocker, but now does not seem like an unreasonable
> time to lower these INFO messages down to DEBUG.

Not a blocker perhaps, but it's better if we can get new behavior to
be more or less right the first time.

regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Thu, 2 May 2019 at 06:18, Sergei Kornilov  wrote:
>
> > I proposed that we didn't need those messages at all, which Robert pushed
> > back on, but I'd be willing to compromise by reducing them to NOTICE or
> > DEBUG level.
>
> I posted patch with such change in a separate topic: 
> https://commitfest.postgresql.org/23/2076/
>
> PS: not think this is blocker for v12

Probably not a blocker, but now does not seem like an unreasonable
time to lower these INFO messages down to DEBUG. If not, then we can
just remove the item from the open items list. I just put it there as
I didn't want it getting forgotten about as it wasn't going to be
anybody's priority to think hard about it on the final few days of the
last commitfest of the release.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hi

> I proposed that we didn't need those messages at all, which Robert pushed
> back on, but I'd be willing to compromise by reducing them to NOTICE or
> DEBUG level.

I posted patch with such change in a separate topic: 
https://commitfest.postgresql.org/23/2076/

PS: not think this is blocker for v12

regards, Sergei

Re: using index or check in ALTER TABLE SET NOT NULL

[Tom Lane]

Andres Freund  writes:
> There's still an open item "Debate INFO messages in ATTACH PARTITION and
> SET NOT NULL" for this thread.  Is there anything further to be done? Or
> are we content with this for v12?

IIRC, that's based on my complaint that these messages have no business
being INFO level.  I'm definitely not content with the current state.

I proposed that we didn't need those messages at all, which Robert pushed
back on, but I'd be willing to compromise by reducing them to NOTICE or
DEBUG level.

The actually intended use of INFO level is to mark messages that the user
actively asked for (via VERBOSE or some morally-equivalent option), which
is why that level has such high priority.  If we had something like a
VERBOSE option for ALTER TABLE, it'd make plenty of sense to follow
VACUUM's precedent:

if (params->options & VACOPT_VERBOSE)
elevel = INFO;
else
elevel = DEBUG2;

It seems a bit late to be inventing such a thing for v12 though.

In the meantime, I'm not very content with random subsets of
ALTER TABLE acting as if they have been granted permission
to be VERBOSE.  It's unfriendly, and it's inconsistent because
there are so many other expensive ALTER TABLE actions that don't
do this.

regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

[Andres Freund]

On 2019-03-25 11:09:47 -0400, Robert Haas wrote:
> On Mon, Mar 25, 2019 at 3:51 AM David Steele  wrote:
> > On 3/17/19 3:40 PM, David Rowley wrote:
> > > On Thu, 14 Mar 2019 at 06:24, Robert Haas  wrote:
> > >
> > > I think we can mark this patch as committed now as I don't think the
> > > lack of a way to test it is likely to cause it to be reverted.
> >
> > Should we mark this as committed now or is there more tweaking to be done?
> 
> I have now marked it as Committed.

There's still an open item "Debate INFO messages in ATTACH PARTITION and
SET NOT NULL" for this thread.  Is there anything further to be done? Or
are we content with this for v12?

- Andres

Re: Re: using index or check in ALTER TABLE SET NOT NULL

[Robert Haas]

On Mon, Mar 25, 2019 at 3:51 AM David Steele  wrote:
> On 3/17/19 3:40 PM, David Rowley wrote:
> > On Thu, 14 Mar 2019 at 06:24, Robert Haas  wrote:
> >
> > I think we can mark this patch as committed now as I don't think the
> > lack of a way to test it is likely to cause it to be reverted.
>
> Should we mark this as committed now or is there more tweaking to be done?

I have now marked it as Committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Re: using index or check in ALTER TABLE SET NOT NULL

[David Steele]

Hi Robert,

On 3/17/19 3:40 PM, David Rowley wrote:

On Thu, 14 Mar 2019 at 06:24, Robert Haas  wrote:

I think we can mark this patch as committed now as I don't think the
lack of a way to test it is likely to cause it to be reverted.


Should we mark this as committed now or is there more tweaking to be done?

Regards,
--
-David
da...@pgmasters.net

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello

> If we don't have this for SET NOT NULL then we should either add it or
> remove the one from ATTACH PARTITION.

I proposed to lower report level to debug1 in a separate thread: 
https://www.postgresql.org/message-id/4859321552643736%40myt5-02b80404fd9e.qloud-c.yandex.net
Possible better would be link it.

regards, Sergei

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Thu, 14 Mar 2019 at 06:24, Robert Haas  wrote:
>
> On Wed, Mar 13, 2019 at 1:09 PM Tom Lane  wrote:
> > Sergei Kornilov  writes:
> > >> Ugh, I guess so. Or how about changing the message itself to use
> > >> INFO, like we already do in QueuePartitionConstraintValidation?
> >
> > > Fine for me. But year ago this was implemented in my patch and Tom voted 
> > > against using INFO level for such purpose: 
> > > https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us
> >
> > What I thought then was that you didn't need the message at all,
> > at any debug level.  I still think that.  It might have been useful
> > for development purposes but it does not belong in committed code.
> > INFO (making it impossible for anybody to not have the message
> > in-their-face) is right out.
>
> I find that position entirely wrong-headed.  If you think users have
> no interest in a message telling them whether their gigantic table is
> getting scanned or not, you're wrong.  Maybe you're right that
> everyone doesn't want it, but I'm positive some do.  We've had
> requests for very similar things on this very mailing list more than
> one.

If we don't have this for SET NOT NULL then we should either add it or
remove the one from ATTACH PARTITION.  I don't think we need to decide
which it is now, so I've added an item to the open items list that
this is out for debate.

https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#Open_Issues

I think we can mark this patch as committed now as I don't think the
lack of a way to test it is likely to cause it to be reverted.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Justin Pryzby]

Hi,

On Wed, Mar 13, 2019 at 01:25:11PM -0400, Robert Haas wrote:
> On Wed, Mar 13, 2019 at 1:19 PM Tom Lane  wrote:
> > Oh, and yes, I think QueuePartitionConstraintValidation's usage
> > is an unacceptable abuse of INFO level.  I'm surprised we haven't
> > gotten complaints about it yet.
> 
> Perhaps that's because users aren't as direly opposed to informational
> messages from DDL commands as you seem to believe...

On Wed, Mar 13, 2019 at 01:09:32PM -0400, Tom Lane wrote:
> Sergei Kornilov  writes:
> >> Ugh, I guess so. Or how about changing the message itself to use
> >> INFO, like we already do in QueuePartitionConstraintValidation?
> 
> > Fine for me. But year ago this was implemented in my patch and Tom voted 
> > against using INFO level for such purpose: 
> > https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us
> 
> What I thought then was that you didn't need the message at all,
> at any debug level.  I still think that.  It might have been useful
> for development purposes but it does not belong in committed code.
> INFO (making it impossible for anybody to not have the message
> in-their-face) is right out.

I'm writing to provide a datapoint for the existing message regarding "implied
by existing constraints" in QueuePartitionConstraintValidation.

On the one hand, I agree that message at INFO is more verbose than other
commands, and I'm 20% surprised by it.

On the other hand, I *like* the output (including at INFO level).  It alerted
me to 1) deficiency with some of our tables (due to column not marked NOT
NULL); and, 2) helped me to understand how to improve some of our dynamic DDL.

If the message weren't visible at LOG, I'd miss it, at least sometimes, and
look for how to re-enable it (but statement logging of DEBUG is beyond
excessive).

For context, I'm someone who configures servers with log_min_messages=info and
who sometimes SETs client_min_messages='DEBUG1' ; I have $toomany warnings in
our monitoring system, because I'd rather see twice as much stuff that may or
may not be interesting than miss 10% of things I needed to see.

Justin

Re: using index or check in ALTER TABLE SET NOT NULL

[Robert Haas]

On Wed, Mar 13, 2019 at 1:19 PM Tom Lane  wrote:
> Oh, and yes, I think QueuePartitionConstraintValidation's usage
> is an unacceptable abuse of INFO level.  I'm surprised we haven't
> gotten complaints about it yet.

Perhaps that's because users aren't as direly opposed to informational
messages from DDL commands as you seem to believe...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

[Robert Haas]

On Wed, Mar 13, 2019 at 1:09 PM Tom Lane  wrote:
> Sergei Kornilov  writes:
> >> Ugh, I guess so. Or how about changing the message itself to use
> >> INFO, like we already do in QueuePartitionConstraintValidation?
>
> > Fine for me. But year ago this was implemented in my patch and Tom voted 
> > against using INFO level for such purpose: 
> > https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us
>
> What I thought then was that you didn't need the message at all,
> at any debug level.  I still think that.  It might have been useful
> for development purposes but it does not belong in committed code.
> INFO (making it impossible for anybody to not have the message
> in-their-face) is right out.

I find that position entirely wrong-headed.  If you think users have
no interest in a message telling them whether their gigantic table is
getting scanned or not, you're wrong.  Maybe you're right that
everyone doesn't want it, but I'm positive some do.  We've had
requests for very similar things on this very mailing list more than
one.

And if you think that it's not useful to have regression tests that
verify that the behavior is correct, well, that's not a question with
one objectively right answer, but I emphatically disagree with that
position.  This behavior is often quite subtle, especially in
combination with partitioned tables where different partitions may get
different treatment.  It would not be at all difficult to break it
without realizing.

I do understand that we seem to have no good way of doing this that
lets users have the option of seeing this message and also makes it
something that can be regression-tested.  INFO is out because there's
no option, and DEBUG1 is out because it doesn't work in the regression
tests.  However, I think giving up and saying "ok, well that's just
how it is, too bad" is, one, letting the tail wag the dog, and two, a
terribly disappointing attitude.

I've just reverted the 0002 portion of the patch, which should
hopefully fix this problem for now.  But I strongly encourage you
think of something to which you could say "yes" instead of just
shooting everything people want to do in this area down.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

[Tom Lane]

I wrote:
> Sergei Kornilov  writes:
>>> Ugh, I guess so. Or how about changing the message itself to use
>>> INFO, like we already do in QueuePartitionConstraintValidation?

>> Fine for me. But year ago this was implemented in my patch and Tom voted 
>> against using INFO level for such purpose: 
>> https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us

> What I thought then was that you didn't need the message at all,
> at any debug level.  I still think that.

Oh, and yes, I think QueuePartitionConstraintValidation's usage
is an unacceptable abuse of INFO level.  I'm surprised we haven't
gotten complaints about it yet.

regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

[Tom Lane]

Sergei Kornilov  writes:
>> Ugh, I guess so. Or how about changing the message itself to use
>> INFO, like we already do in QueuePartitionConstraintValidation?

> Fine for me. But year ago this was implemented in my patch and Tom voted 
> against using INFO level for such purpose: 
> https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us

What I thought then was that you didn't need the message at all,
at any debug level.  I still think that.  It might have been useful
for development purposes but it does not belong in committed code.
INFO (making it impossible for anybody to not have the message
in-their-face) is right out.

regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hi

> Ugh, I guess so. Or how about changing the message itself to use
> INFO, like we already do in QueuePartitionConstraintValidation?

Fine for me. But year ago this was implemented in my patch and Tom voted 
against using INFO level for such purpose: 
https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us

regards, Sergei

Re: using index or check in ALTER TABLE SET NOT NULL

[Robert Haas]

On Wed, Mar 13, 2019 at 11:17 AM Sergei Kornilov  wrote:
> > The buildfarm thinks additional nitpicking is needed.
>
> hm. Patch was committed with debug1 level tests and many animals uses 
> log_statement = 'all'. Therefore they have additional line in result: LOG:  
> statement: alter table pg_class alter column relname drop not null; and 
> similar for other queries.
> I think we better would be to revert "set client_min_messages to 'debug1';"  
> part.

Ugh, I guess so.  Or how about changing the message itself to  use
INFO, like we already do in QueuePartitionConstraintValidation?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hi

> The buildfarm thinks additional nitpicking is needed.

hm. Patch was committed with debug1 level tests and many animals uses 
log_statement = 'all'. Therefore they have additional line in result: LOG:  
statement: alter table pg_class alter column relname drop not null; and similar 
for other queries.
I think we better would be to revert "set client_min_messages to 'debug1';"  
part.

regards, Sergei

Re: using index or check in ALTER TABLE SET NOT NULL

[Tom Lane]

Robert Haas  writes:
> On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov  wrote:
>> Agreed, in attached new version ...

> Committed with a little more nitpicking.

The buildfarm thinks additional nitpicking is needed.

regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Wow, thank you!

13.03.2019, 15:57, "Robert Haas" :
> On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov  wrote:
>>  Agreed, in attached new version ...
>
> Committed with a little more nitpicking.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

[Robert Haas]

On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov  wrote:
> Agreed, in attached new version ...

Committed with a little more nitpicking.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello

> Dispatches from the department of grammatical nitpicking...

Thank you!

> + entire table, however if a valid CHECK constraint is
>
> I think this should be:
>
> entire table; however, if...
>
> + * are set NOT NULL, however, if we can find a constraint which proves
>
> similarly here

Changed

> + ereport(DEBUG1,
> + (errmsg("verifying table \"%s\" NOT NULL constraint "
> + "on %s attribute by existed constraints",
> + RelationGetRelationName(rel), NameStr(attr->attname;
>
> Ugh, that doesn't read well at all. How about:
>
> existing constraints on column "%s"."%s" are sufficient to prove that
> it does not contain nulls

Changed

> - * in implicit-AND form.
> + * in implicitly-AND form, must only contain immutable clauses
> + * and all Vars must be varno=1.
>
> I think you should leave the existing sentence alone (implicit-AND is
> correct, implicitly-AND is not) and add a new sentence that says the
> stuff you want to add.

Ok

> + * "Existing constraints" include its check constraints and optional
> + * caller-provided existConstraint list. existConstraint list is modified
> + * during ConstraintImpliedByRelConstraint call and would represent all
> + * assumed conditions. testConstraint describes the constraint to validate.
> + * Both existConstraint and testConstraint must be in implicitly-AND form,
> + * must only contain immutable clauses and all Vars must be varno=1.
>
> I think that it might be better to copy the list rather than to have
> the comment note that it gets mutated, but regardless the grammar
> needs improvement here.

Agreed, in attached new version I copy the list and do not modify parameter.

> I object to removing this.

Okay, I revert this David's change

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 890b23afd6..cda0ea8972 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -214,8 +214,17 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  SET NOT NULL may only be applied to a column
+  providing none of the records in the table contain a
+  NULL value for the column.  Ordinarily this is
+  checked during the ALTER TABLE by scanning the
+  entire table; however if a valid CHECK constraint is
+  found which proves no NULL can exist, then the
+  table scan is skipped.
  
 
  
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5ed560b02f..15407778b4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -160,7 +160,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -372,6 +372,9 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
+static bool ConstraintImpliedByRelConstraint(Relation scanrel,
+	 List *partConstraint, List *existedConstraints);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4550,10 +4553,11 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 		else
 		{
 			/*
-			 * Test the current data within the table against new constraints
-			 * generated by ALTER TABLE commands, but don't rebuild data.
+			 * If required, test the current data within the table against new
+			 * constraints generated by ALTER TABLE commands, but don't rebuild
+			 * data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4714,13 +4718,13 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
-		 * If we are rebuilding the tuples OR if we added any new NOT NULL
-		 * constraints, check all not-null 

Re: using index or check in ALTER TABLE SET NOT NULL

[Robert Haas]

On Sun, Mar 10, 2019 at 11:30 PM David Rowley
 wrote:
> Looks good to me.  Good idea to keep the controversial setting of
> client_min_messages to debug1 in the tests in a separate patch.
>
> Apart from a few lines that need to be wrapped at 80 chars and some
> comment lines that seem to have been wrapped early, I'm happy for it
> to be marked as ready for committer, but I'll defer to Ildar in case
> he wants to have another look.

Dispatches from the department of grammatical nitpicking...

+  entire table, however if a valid CHECK constraint is

I think this should be:

entire table; however, if...

+ * are set NOT NULL, however, if we can find a constraint which proves

similarly here

+ ereport(DEBUG1,
+ (errmsg("verifying table \"%s\" NOT NULL constraint "
+ "on %s attribute by existed constraints",
+ RelationGetRelationName(rel), NameStr(attr->attname;

Ugh, that doesn't read well at all.  How about:

existing constraints on column "%s"."%s" are sufficient to prove that
it does not contain nulls

- * in implicit-AND form.
+ * in implicitly-AND form, must only contain immutable clauses
+ * and all Vars must be varno=1.

I think you should leave the existing sentence alone (implicit-AND is
correct, implicitly-AND is not) and add a new sentence that says the
stuff you want to add.

+ * "Existing constraints" include its check constraints and optional
+ * caller-provided existConstraint list. existConstraint list is modified
+ * during ConstraintImpliedByRelConstraint call and would represent all
+ * assumed conditions. testConstraint describes the constraint to validate.
+ * Both existConstraint and testConstraint must be in implicitly-AND form,
+ * must only contain immutable clauses and all Vars must be varno=1.

I think that it might be better to copy the list rather than to have
the comment note that it gets mutated, but regardless the grammar
needs improvement here.  Maybe: On entry, existConstraint is a
caller-provided list of conditions which this function may assume to
be true; on exit, it will have been destructively modified by the
addition of the table's CHECK constraints.  testConstraint is the
constraint to validate.  Both existConstraint and testConstraint must
be in implicit-AND form, must only contain immutable clauses, and must
contain only Vars with varno = 1.

- * not-false and try to prove the same for partConstraint.
- *
- * Note that predicate_implied_by assumes its first argument is known
- * immutable.  That should always be true for partition constraints, so we
- * don't test it here.
+ * not-false and try to prove the same for testConstraint.

I object to removing this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Mon, 11 Mar 2019 at 00:13, Sergei Kornilov  wrote:
>
> > Providing I'm imagining it correctly, I do think the patch is slightly
> > cleaner with that change.
>
> Ok, sounds reasonable. I changed patch this way.

Looks good to me.  Good idea to keep the controversial setting of
client_min_messages to debug1 in the tests in a separate patch.

Apart from a few lines that need to be wrapped at 80 chars and some
comment lines that seem to have been wrapped early, I'm happy for it
to be marked as ready for committer, but I'll defer to Ildar in case
he wants to have another look.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hi

> Providing I'm imagining it correctly, I do think the patch is slightly
> cleaner with that change.

Ok, sounds reasonable. I changed patch this way.

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 890b23afd6..926b3361ea 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -214,8 +214,17 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  SET NOT NULL may only be applied to a column
+  providing none of the records in the table contain a
+  NULL value for the column.  Ordinarily this is
+  checked during the ALTER TABLE by scanning the
+  entire table, however if a valid CHECK constraint is
+  found which proves no NULL can exist, then the
+  table scan is skipped.
  
 
  
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 59341e2a40..5200aac530 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -160,7 +160,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -372,6 +372,9 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
+static bool ConstraintImpliedByRelConstraint(Relation scanrel,
+	 List *partConstraint, List *existedConstraints);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4550,10 +4553,11 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 		else
 		{
 			/*
-			 * Test the current data within the table against new constraints
-			 * generated by ALTER TABLE commands, but don't rebuild data.
+			 * If required, test the current data within the table against new
+			 * constraints generated by ALTER TABLE commands, but don't rebuild
+			 * data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4714,13 +4718,13 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
-		 * If we are rebuilding the tuples OR if we added any new NOT NULL
-		 * constraints, check all not-null constraints.  This is a bit of
-		 * overkill but it minimizes risk of bugs, and heap_attisnull is a
-		 * pretty cheap test anyway.
+		 * If we are rebuilding the tuples OR if we added any new but not
+		 * verified NOT NULL constraints, check all not-null constraints.
+		 * This is a bit of overkill but it minimizes risk of bugs, and
+		 * heap_attisnull is a pretty cheap test anyway.
 		 */
 		for (i = 0; i < newTupDesc->natts; i++)
 		{
@@ -5726,11 +5730,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		{
 			/*
 			 * If the new column is NOT NULL, and there is no missing value,
-			 * tell Phase 3 it needs to test that. (Note we don't do this for
-			 * an OID column.  OID will be marked not null, but since it's
-			 * filled specially, there's no need to test anything.)
+			 * tell Phase 3 it needs to check for NULLs.
 			 */
-			tab->new_notnull |= colDef->is_not_null;
+			tab->verify_new_notnull |= colDef->is_not_null;
 		}
 	}
 
@@ -6098,8 +6100,19 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		/*
+		 * Ordinarily phase 3 must ensure that no NULLs exist in columns that
+		 * are set NOT NULL, however, if we can find a constraint which proves
+		 * this then we can skip that.  However, we needn't bother looking if
+		 * we've already found that we must verify some other NOT NULL
+		 * constraint.
+		 */
+		if (!tab->verify_new_notnull &&
+			

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Wed, 6 Mar 2019 at 08:41, Sergei Kornilov  wrote:
> In this case we need extra argument for ConstraintImpliedByRelConstraint for 
> some caller-provided existConstraint, right? Along with Relation itself? Then 
> I need make copy of existConstraint, append relation constraints and call 
> predicate_implied_by. If I understood correctly pseudocode would be:
>
> PartConstraintImpliedByRelConstraint(rel, testConstraint)
>   generate notNullConstraint from NOT NULL table attributes
>   call ConstraintImpliedByRelConstraint(rel, testConstraint, 
> notNullConstraint)
>
> ConstraintImpliedByRelConstraint(rel, testConstraint, existsConstraint)
>   copy existsConstraint to localExistsConstraint variable
>   append relation constraints to localExistsConstraint list
>   call predicate_implied_by
>
> I thing redundant IS NOT NULL check is not issue here and we not need extra 
> arguments for ConstraintImpliedByRelConstraint. Was not changed in attached 
> version, but I will change if you think this would be better design.

I was really just throwing the idea out there for review. I don't
think that I'm insisting on it.

FWIW I think you could get away without the copy of the constraint
providing it was documented that the list is modified during the
ConstraintImpliedByRelConstraint call and callers must make a copy
themselves if they need an unmodified version. Certainly,
PartConstraintImpliedByRelConstraint won't need to make a copy, so
there's probably not much reason to assume that possible future
callers will.

Providing I'm imagining it correctly, I do think the patch is slightly
cleaner with that change.

It's:
a) slightly more efficient due to not needlessly checking a bunch of
IS NOT NULLs (imagine a 1000 column table with almost all NOT NULL and
a single CHECK constraint); and
b) patch has a smaller footprint (does not modify existing callers of
PartConstraintImpliedByRelConstraint()); and
c) does not modify an existing API function.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello, David!

> I've made a pass over v10. I think it's in pretty good shape, but I
> did end up changing a few small things.

Thank you! I merged your changes to new patch version.

> The only thing that I'm a bit unsure of is the tests. I've read the
> thread and I see the discussion above about it. I'd personally have
> thought INFO was fine since ATTACH PARTITION does that, but I see
> objections.

It is unclear for me if we have consensus about INFO ereport in ATTACH 
PARTITION.

> It appears that all the tests just assume that the CHECK
> constraint was used to validate the SET NOT NULL. I'm not all that
> sure if there's any good reason not to set client_min_messages =
> 'debug1' just before your test then RESET client_min_messages;
> afterwards. No other tests seem to do it, but my only thoughts on the
> reason not to would be that it might fail if someone added another
> debug somewhere, but so what? they should update the expected results
> too.

Not sure we have consensus about debug messages in tests, so I did such test 
changes in additional patch.

> separate that part out and
> put back the function named PartConstraintImpliedByRelConstraint and
> have it do the IS NOT NULL building before calling
> ConstraintImpliedByRelConstraint(). No duplicate code that way and you
> also don't need to touch partbound.c

In this case we need extra argument for ConstraintImpliedByRelConstraint for 
some caller-provided existConstraint, right? Along with Relation itself? Then I 
need make copy of existConstraint, append relation constraints and call 
predicate_implied_by. If I understood correctly pseudocode would be:

PartConstraintImpliedByRelConstraint(rel, testConstraint)
  generate notNullConstraint from NOT NULL table attributes
  call ConstraintImpliedByRelConstraint(rel, testConstraint, notNullConstraint)

ConstraintImpliedByRelConstraint(rel, testConstraint, existsConstraint)
  copy existsConstraint to localExistsConstraint variable
  append relation constraints to localExistsConstraint list
  call predicate_implied_by
  
I thing redundant IS NOT NULL check is not issue here and we not need extra 
arguments for ConstraintImpliedByRelConstraint. Was not changed in attached 
version, but I will change if you think this would be better design.

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 890b23afd6..926b3361ea 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -214,8 +214,17 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  SET NOT NULL may only be applied to a column
+  providing none of the records in the table contain a
+  NULL value for the column.  Ordinarily this is
+  checked during the ALTER TABLE by scanning the
+  entire table, however if a valid CHECK constraint is
+  found which proves no NULL can exist, then the
+  table scan is skipped.
  
 
  
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a93b13c2fe..b03e1bc875 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -158,7 +158,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -370,6 +370,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4506,10 +4507,11 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 		else
 		{
 			/*
-			 * Test the current data within the table against new constraints
-			 * generated by ALTER TABLE commands, but don't rebuild data.
+			 * If required, test the current data within the table against new
+			 * constraints generated by ALTER TABLE commands, but don't rebuild
+			 * data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != 

Re: using index or check in ALTER TABLE SET NOT NULL

[David Rowley]

On Sun, 15 Apr 2018 at 19:09, Sergei Kornilov  wrote:
> Attached updated patch follows recent Reorganize partitioning code commit.

I've made a pass over v10. I think it's in pretty good shape, but I
did end up changing a few small things.

1. Tweaked the documents a bit. I was going to just change "Full table
scan" to "A full table scan", but I ended up rewriting the entire
paragraph.
2. In ATRewriteTables, updated comment to mention "If required" since
the scan may not be required if SET NOT NULLs are already proved okay.
3. Updated another forgotten comment in ATRewriteTables.
4. Removed mention about table's with OIDs in ATExecAddColumn(). This
seems left over from 578b229718.
5. Changed ATExecSetNotNull not to check for matching constraints if
we're already going to make a pass over the table. Setting
verify_new_notnull to true again does not make it any more true. :)
6. Tweaked NotNullImpliedByRelConstraints() a bit. You were calling
lappend on an empty list. make_list1() is fine. Don't need a variable
for that, just pass it to the function.
7. Tightened up the comments in ConstraintImpliedByRelConstraint() to
mention that it only supports immutable quals containing vars with
varno=1. Removed the comment near the bottom that mentioned that in
passing.

The only thing that I'm a bit unsure of is the tests. I've read the
thread and I see the discussion above about it.  I'd personally have
thought INFO was fine since ATTACH PARTITION does that, but I see
objections.  It appears that all the tests just assume that the CHECK
constraint was used to validate the SET NOT NULL. I'm not all that
sure if there's any good reason not to set client_min_messages =
'debug1' just before your test then RESET client_min_messages;
afterwards.  No other tests seem to do it, but my only thoughts on the
reason not to would be that it might fail if someone added another
debug somewhere, but so what? they should update the expected results
too.

Oh, one other thing I don't really like is that
ConstraintImpliedByRelConstraint() adds all of the other column's IS
NOT NULL constraints to the existConstraint. This is always wasted
effort for the SET NOT NULL use case.  I wonder if a new flag should
be added to control this, or even better, separate that part out and
put back the function named PartConstraintImpliedByRelConstraint and
have it do the IS NOT NULL building before calling
ConstraintImpliedByRelConstraint(). No duplicate code that way and you
also don't need to touch partbound.c

Setting this on waiting on author.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


alter_table_set_not_null_by_constraints_only_v10_fixes.patch
Description: Binary data

Re: using index or check in ALTER TABLE SET NOT NULL

[Dmitry Dolgov]

> On Thu, Nov 22, 2018 at 6:04 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> Absolutely agree. Looking at the history of the patch I see that it went
> through some extensive review and even was marked as Ready for Committer 
> before
> the commentary from Peter, but since then some changes were made (rebase and
> code reorganization). Can someone from the reviewers confirm (or not) if the
> patch is in a good shape? If no, then I'll try to allocate some time for that,
> but can't promise any exact date.

Ok, I've reviewed this patch a bit. I don't see any significant problems with
the code itself, but to be honest I've got an impression that simplicity of
this patch is misleading and it covers too narrow use case. E.g. one can do
something equal to SET NOT NULL, like adding a new constraint

CREATE TABLE test(data text, CHECK(data IS NOT NULL));
ALTER TABLE test ADD CONTSTRAINT data_chk CHECK (data is not null);

or use not null domain

CREATE DOMAIN not_null AS text CHECK(VALUE IS NOT NULL);
CREATE TABLE test(data not_null);
ALTER TABLE test ALTER COLUMN data SET NOT NULL;

and it still leads to a full table scan. For foreign tables the control flow
jump over NotNullImpliedByRelConstraints, so this logic is never checked. I'm
not sure if taking care of mentioned examples is enough (maybe there are more
of them), or the patch needs to suggest some more general solution here.

Of course there is a possibility that improvement even in this form for this
narrow use case is better than potentially more general approach in the future.
Any opinions about this?

Re: using index or check in ALTER TABLE SET NOT NULL

[Dmitry Dolgov]

> On Tue, Nov 13, 2018 at 1:59 PM Alvaro Herrera  
> wrote:
>
> On 2018-Nov-13, Dmitry Dolgov wrote:
>
> > > On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov  wrote:
> > >
> > > > If not properly cataloguing NOT NULL constraints would be fixed, can it
> > > > potentially conflict with the current patch or not?
> > > We already doing same stuff for "alter table attach partition" and in 
> > > this patch i use exactly this routine. If proper cataloguing would 
> > > conflict with my patch - it would conflict with "attach partition" 
> > > validation too.
> > > I think proper cataloguing can be implemented without conflict with 
> > > proposed feature.
> >
> > Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint.
> > Then maybe it makes sense to go with the solution, proposed in this thread,
> > while leaving open the possibility of having "SET NOT NULL NOT VALID"? From
> > the functionality point of view it definitely would be beneficial. Any other
> > opinions?
>
> I think we should ignore any SET NOT NULL NOT VALID patches, because
> in practice they don't exist.  Let's move forward with this, because the
> optimization being proposed is clearly very useful.

Absolutely agree. Looking at the history of the patch I see that it went
through some extensive review and even was marked as Ready for Committer before
the commentary from Peter, but since then some changes were made (rebase and
code reorganization). Can someone from the reviewers confirm (or not) if the
patch is in a good shape? If no, then I'll try to allocate some time for that,
but can't promise any exact date.

Re: using index or check in ALTER TABLE SET NOT NULL

[Alvaro Herrera]

On 2018-Nov-13, Dmitry Dolgov wrote:

> > On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov  wrote:
> >
> > > If not properly cataloguing NOT NULL constraints would be fixed, can it
> > > potentially conflict with the current patch or not?
> > We already doing same stuff for "alter table attach partition" and in this 
> > patch i use exactly this routine. If proper cataloguing would conflict with 
> > my patch - it would conflict with "attach partition" validation too.
> > I think proper cataloguing can be implemented without conflict with 
> > proposed feature.
> 
> Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint.
> Then maybe it makes sense to go with the solution, proposed in this thread,
> while leaving open the possibility of having "SET NOT NULL NOT VALID"? From
> the functionality point of view it definitely would be beneficial. Any other
> opinions?

I think we should ignore any SET NOT NULL NOT VALID patches, because
in practice they don't exist.  Let's move forward with this, because the
optimization being proposed is clearly very useful.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Dmitry Dolgov]

> On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov  wrote:
>
> > If not properly cataloguing NOT NULL constraints would be fixed, can it
> > potentially conflict with the current patch or not?
> We already doing same stuff for "alter table attach partition" and in this 
> patch i use exactly this routine. If proper cataloguing would conflict with 
> my patch - it would conflict with "attach partition" validation too.
> I think proper cataloguing can be implemented without conflict with proposed 
> feature.

Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint.
Then maybe it makes sense to go with the solution, proposed in this thread,
while leaving open the possibility of having "SET NOT NULL NOT VALID"? From
the functionality point of view it definitely would be beneficial. Any other
opinions?

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello

> This patch went through the last tree commit fests without any noticeable 
> activity
Well, last two CF. During march commitfest patch has activity and was marked as 
ready for committer. But at end of july CF was no further activity and patch 
was reverted back to "need review" with reason [1]:
> It's not clear that there is consensus that this is wanted.
I can't say something here.

> If not properly cataloguing NOT NULL constraints would be fixed, can it
> potentially conflict with the current patch or not?
We already doing same stuff for "alter table attach partition" and in this 
patch i use exactly this routine. If proper cataloguing would conflict with my 
patch - it would conflict with "attach partition" validation too.
I think proper cataloguing can be implemented without conflict with proposed 
feature.

regards, Sergei

[1]: 
https://www.postgresql.org/message-id/c4c7556d-a046-ac29-2549-bdef0078b6fe%402ndQuadrant.com

Re: using index or check in ALTER TABLE SET NOT NULL

[Dmitry Dolgov]

>On Sun, 15 Apr 2018 at 09:09, Sergei Kornilov  wrote:
>
> Attached updated patch follows recent Reorganize partitioning code commit.
> regards, Sergei

This patch went through the last tree commit fests without any noticeable
activity, but cfbot says it still applies and doesn't break any tests. The
patch itself is rather small, the only significant objection I see from the
thread is that probably "SET NOT NULL NOT VALID" feature could be more
appropriate way of fixing the original problem, but looks like no one is
working on that (the only related thread I could found was this one [1]). If
not properly cataloguing NOT NULL constraints would be fixed, can it
potentially conflict with the current patch or not?


[1]: 
https://www.postgresql.org/message-id/flat/CAASwCXcOokBqHf8BXECbDgOLkXxJyL_Q_twhg2dGWSvgMzz%3DxA%40mail.gmail.com

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello
Attached updated patch follows recent Reorganize partitioning code commit.
regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index bd22627..db98a98 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -215,8 +215,15 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  You can only use SET NOT NULL when the column 
+  contains no null values. Full table scan is performed to check 
+  this condition unless there are valid CHECK 
+  constraints that prohibit NULL values for specified column.
+  In the latter case table scan is skipped.
  
 
  
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c1a9bda..164c148 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -162,7 +162,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -374,6 +374,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4465,7 +4466,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4626,7 +4627,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5661,7 +5662,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			 * an OID column.  OID will be marked not null, but since it's
 			 * filled specially, there's no need to test anything.)
 			 */
-			tab->new_notnull |= colDef->is_not_null;
+			tab->verify_new_notnull |= colDef->is_not_null;
 		}
 	}
 
@@ -6065,8 +6066,17 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		/*
+		 * Verifying table data can be done in Phase 3 with single table scan
+		 * for all constraint (both existing and new)
+		 * We can skip this check only if every new NOT NULL contraint
+		 * implied by existed table constraints
+		 */
+		if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+		{
+			/* Tell Phase 3 it needs to test the constraints */
+			tab->verify_new_notnull = true;
+		}
 
 		ObjectAddressSubSet(address, RelationRelationId,
 			RelationGetRelid(rel), attnum);
@@ -6083,6 +6093,46 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 }
 
 /*
+ * NotNullImpliedByRelConstraints
+ *		Does rel's existing constraints imply NOT NULL for given attribute
+ */
+static bool
+NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr)
+{
+	List	   *notNullConstraint = NIL;
+	NullTest   *nnulltest = makeNode(NullTest);
+
+	nnulltest->arg = (Expr *) makeVar(1,
+	  attr->attnum,
+	  attr->atttypid,
+	  attr->atttypmod,
+	  attr->attcollation,
+	  0);
+	nnulltest->nulltesttype = IS_NOT_NULL;
+
+	/*
+	 * same thing as in ConstraintImpliedByRelConstraint argisrow=false is
+	 * correct even for a composite column, because attnotnull does not
+	 * represent a SQL-spec IS NOT NULL test in such a case, just IS DISTINCT
+	 * FROM NULL.
+	 */
+	nnulltest->argisrow = false;
+	nnulltest->location = -1;
+	notNullConstraint = lappend(notNullConstraint, nnulltest);
+
+	if (ConstraintImpliedByRelConstraint(rel, 

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hi
I noticed new merge conflict, updated version attached.

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index bd22627..db98a98 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -215,8 +215,15 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  You can only use SET NOT NULL when the column 
+  contains no null values. Full table scan is performed to check 
+  this condition unless there are valid CHECK 
+  constraints that prohibit NULL values for specified column.
+  In the latter case table scan is skipped.
  
 
  
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 0f5932f..6fd3663 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1153,7 +1153,7 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 	 * not contain any row that would belong to the new partition, we can
 	 * avoid scanning the default partition.
 	 */
-	if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
+	if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
 	{
 		ereport(INFO,
 (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
@@ -1197,8 +1197,8 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 			 * that it will not contain any row that would belong to the new
 			 * partition, we can avoid scanning the child table.
 			 */
-			if (PartConstraintImpliedByRelConstraint(part_rel,
-	 def_part_constraints))
+			if (ConstraintImpliedByRelConstraint(part_rel,
+ def_part_constraints))
 			{
 ereport(INFO,
 		(errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f810885..e10edb1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -160,7 +160,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -372,6 +372,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4461,7 +4462,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4622,7 +4623,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5657,7 +5658,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			 * an OID column.  OID will be marked not null, but since it's
 			 * filled specially, there's no need to test anything.)
 			 */
-			tab->new_notnull |= colDef->is_not_null;
+			tab->verify_new_notnull |= colDef->is_not_null;
 		}
 	}
 
@@ -6061,8 +6062,17 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		/*
+		 * Verifying table data can be done in Phase 3 with single table scan
+		 * for all constraint (both existing and new)
+		 * We can skip this check only if every new NOT NULL contraint
+		 * implied by existed table constraints
+		 */
+		if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+	

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello
I notice my patch does not apply again. Here is update to current HEAD

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index bd22627..db98a98 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -215,8 +215,15 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  You can only use SET NOT NULL when the column 
+  contains no null values. Full table scan is performed to check 
+  this condition unless there are valid CHECK 
+  constraints that prohibit NULL values for specified column.
+  In the latter case table scan is skipped.
  
 
  
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 0f5932f..6fd3663 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1153,7 +1153,7 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 	 * not contain any row that would belong to the new partition, we can
 	 * avoid scanning the default partition.
 	 */
-	if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
+	if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
 	{
 		ereport(INFO,
 (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
@@ -1197,8 +1197,8 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 			 * that it will not contain any row that would belong to the new
 			 * partition, we can avoid scanning the child table.
 			 */
-			if (PartConstraintImpliedByRelConstraint(part_rel,
-	 def_part_constraints))
+			if (ConstraintImpliedByRelConstraint(part_rel,
+ def_part_constraints))
 			{
 ereport(INFO,
 		(errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 43b2fce..11dc92c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -160,7 +160,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -372,6 +372,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4462,7 +4463,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4623,7 +4624,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5658,7 +5659,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			 * an OID column.  OID will be marked not null, but since it's
 			 * filled specially, there's no need to test anything.)
 			 */
-			tab->new_notnull |= colDef->is_not_null;
+			tab->verify_new_notnull |= colDef->is_not_null;
 		}
 	}
 
@@ -6062,8 +6063,17 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		/*
+		 * Verifying table data can be done in Phase 3 with single table scan
+		 * for all constraint (both existing and new)
+		 * We can skip this check only if every new NOT NULL contraint
+		 * implied by existed table constraints
+		 */
+		if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) 

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello, Peter!
I agree my patch seems as strange workaround. And better will be SET NOT NULL 
NOT VALID feature. But this change is very complicated for me
For now my patch is small, simple and provides way for users.

regards, Sergei

06.04.2018, 06:29, "Peter Eisentraut" :
> On 11/29/17 10:52, Sergei Kornilov wrote:
>>  My target problem of adding NOT NULL to big relation without long downtime 
>> can be done with ADD CONSTRAINT NOT VALID, VALIDATE it in second 
>> transaction, then SET NOT NULL by my patch and drop unneeded constraint.
>
> It seems to me that this is a workaround for not properly cataloguing
> NOT NULL constraints. If we fixed that, you could do SET NOT NULL NOT
> VALID and go from there. Maybe we should look again into fixing that.
> That would solve so many problems.
>
> --
> Peter Eisentraut http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Peter Eisentraut]

On 11/29/17 10:52, Sergei Kornilov wrote:
> My target problem of adding NOT NULL to big relation without long downtime 
> can be done with ADD CONSTRAINT NOT VALID, VALIDATE it in second transaction, 
> then SET NOT NULL by my patch and drop unneeded constraint.

It seems to me that this is a workaround for not properly cataloguing
NOT NULL constraints.  If we fixed that, you could do SET NOT NULL NOT
VALID and go from there.  Maybe we should look again into fixing that.
That would solve so many problems.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Ildar Musin]

Hello Sergei,

On 10.03.2018 12:35, Sergei Kornilov wrote:

Hello My patch does not apply after commit
5748f3a0aa7cf78ac6979010273bd9d50869bb8e. Here is update to current
master. Not null constraint is immutable too, so here is no changes
in PartConstraintImpliedByRelConstraint excepts rename and comments
fix.

In this patch version i also revert tests to v4 state: i use DEBUG
ereport instead INFO and code path not tested. Please tell me if i
must change tests some way.

regards, Sergei



Ok, I can't think of any other ways to test it so I have to agree with
Tom Lane i.e. rely only on coverage. (There also were another suggestion
to use statistics [select seq_scan from pg_stat_user_tables where
relid='test'::regclass] which show number of table scans. But statistics
is collected by stat collector process with some latency and hence
cannot be reliable for tests).

Patch seems correct to me, it applies and compiles cleanly, docs
compiles as well, tests pass. Changed status to Ready for Committer.

--
Ildar Musin
i.mu...@postgrespro.ru

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello
My patch does not apply after commit 5748f3a0aa7cf78ac6979010273bd9d50869bb8e. 
Here is update to current master. Not null constraint is immutable too, so here 
is no changes in PartConstraintImpliedByRelConstraint excepts rename and 
comments fix.

In this patch version i also revert tests to v4 state: i use DEBUG ereport 
instead INFO and code path not tested. Please tell me if i must change tests 
some way.

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index afe2139..783a0ef 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -215,8 +215,15 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  You can only use SET NOT NULL when the column 
+  contains no null values. Full table scan is performed to check 
+  this condition unless there are valid CHECK 
+  constraints that prohibit NULL values for specified column.
+  In the latter case table scan is skipped.
  
 
  
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index fcf7655..4fec36d 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1258,7 +1258,7 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 	 * not contain any row that would belong to the new partition, we can
 	 * avoid scanning the default partition.
 	 */
-	if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
+	if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
 	{
 		ereport(INFO,
 (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
@@ -1302,8 +1302,8 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 			 * that it will not contain any row that would belong to the new
 			 * partition, we can avoid scanning the child table.
 			 */
-			if (PartConstraintImpliedByRelConstraint(part_rel,
-	 def_part_constraints))
+			if (ConstraintImpliedByRelConstraint(part_rel,
+ def_part_constraints))
 			{
 ereport(INFO,
 		(errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e559afb..b6d395b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -162,7 +162,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -377,6 +377,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4371,7 +4372,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4532,7 +4533,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5545,7 +5546,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		 * null, but since it's filled specially, there's no need to test
 		 * anything.)
 		 */
-		tab->new_notnull |= colDef->is_not_null;
+		tab->verify_new_notnull |= colDef->is_not_null;
 	}
 
 	/*
@@ -5948,8 +5949,17 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		/*
+		 * 

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello all!
Summarizing, how i must update tests?
We want test real skip of verify phase or only result correctness?

I can verify what i do not break alter table by tests in v4 of my patch. But 
here is no check code path. So my feature may be broken in future without test 
fail. alter table will be correct regardless this feature.
Or i must test code path too? I understood Tom and will remove INFO ereport 
from patch anyway. But currently i have no idea how check code path. As 
mentioned earlier using debug ereport is uncomfortable too. Here is no suitable 
event trigger to track what exactly happens with table. Adding new one for test 
purposes seems overkill.

regards, Sergei

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello

> 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.
I not known. In v4 i use DEBUG1 message and do not check code path in tests at 
all: by full table scan or by constraint, i tested only command result to not 
break behavior.
Today Ildar Musin proposed to test code path through 
NotNullImpliedByRelConstraints function. This is my first patch and I do not 
have the confidence to write a test. So I looked more closely at the alter 
table tests, found several info in attach partition and updated my patch.

> I did not see any INFO messages in a quick test of ALTER TABLE ATTACH
> PARTITION, but if there are any lurking in there, they probably need
> to be downgraded.
In src/test/regress/expected/alter_table.out i found 7 test with
> INFO: partition constraint for table "..." is implied by existing constraints
and 5 with
> INFO:  updated partition constraint for default partition "..." is implied by 
> existing constraints
ereport's are in ValidatePartitionConstraints function 
src/backend/commands/tablecmds.c

regards, Sergei

Re: using index or check in ALTER TABLE SET NOT NULL

[Tom Lane]

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

> I agree and this is reason why i not used DEBUG message in tests as was 
> proposed. I found INFO messages in tests and decided that this was an 
> acceptable option year ago.

INFO is for user-facing messages, such as output from VACUUM VERBOSE,
where the user has specifically requested the output.  I do not think
this falls into that category.  Even if you persuade some committer
to commit it like that, there will be user pushback asking us to get
this noise out of their faces ... and then we'll have to find some
other way to test it.

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.

> In this case we need remove INFO from ALTER TABLE ATTACH PARTITION code and 
> tests. I think is a bad idea of using different rules for same stuff, right? 
> Probably i can do this work too.

I did not see any INFO messages in a quick test of ALTER TABLE ATTACH
PARTITION, but if there are any lurking in there, they probably need
to be downgraded.

regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello

> 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.
Here is no table rewrite, only verify. So here EventTriggerTableRewrite is not 
called. Or i missed something?

> ISTM that depending on DEBUG messages is bad because debugging lines
> added elsewhere will make your tests fail;
I agree and this is reason why i not used DEBUG message in tests as was 
proposed. I found INFO messages in tests and decided that this was an 
acceptable option year ago.

> and INFO is generally frowned upon (I, for one, would frown upon an INFO 
> message raised by ALTER
> TABLE, for sure.) so let's not do that either.
In this case we need remove INFO from ALTER TABLE ATTACH PARTITION code and 
tests. I think is a bad idea of using different rules for same stuff, right? 
Probably i can do this work too.

But it is necessary to decide how the test should look.

regards, Sergei

Re: using index or check in ALTER TABLE SET NOT NULL

[Alvaro Herrera]

Sergei Kornilov wrote:
> Hello, Ildar
> Thanks. I looked ATTACH PARTITION tests and found such checks. If no one is 
> against i will just use in my patch INFO level instead DEBUG1, similarly 
> ATTACH PARTITION code. Updated patch attached.
> 
> Or i can rewrite tests to use DEBUG1 level.

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.)

ISTM that depending on DEBUG messages is bad because debugging lines
added elsewhere will make your tests fail; and INFO is generally frowned
upon (I, for one, would frown upon an INFO message raised by ALTER
TABLE, for sure.) so let's not do that either.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello, Ildar
Thanks. I looked ATTACH PARTITION tests and found such checks. If no one is 
against i will just use in my patch INFO level instead DEBUG1, similarly ATTACH 
PARTITION code. Updated patch attached.

Or i can rewrite tests to use DEBUG1 level.

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index afe2139..783a0ef 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -215,8 +215,15 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  You can only use SET NOT NULL when the column 
+  contains no null values. Full table scan is performed to check 
+  this condition unless there are valid CHECK 
+  constraints that prohibit NULL values for specified column.
+  In the latter case table scan is skipped.
  
 
  
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index fcf7655..4fec36d 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1258,7 +1258,7 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 	 * not contain any row that would belong to the new partition, we can
 	 * avoid scanning the default partition.
 	 */
-	if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
+	if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
 	{
 		ereport(INFO,
 (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
@@ -1302,8 +1302,8 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 			 * that it will not contain any row that would belong to the new
 			 * partition, we can avoid scanning the child table.
 			 */
-			if (PartConstraintImpliedByRelConstraint(part_rel,
-	 def_part_constraints))
+			if (ConstraintImpliedByRelConstraint(part_rel,
+ def_part_constraints))
 			{
 ereport(INFO,
 		(errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 74e020b..c35539a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -162,7 +162,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -377,6 +377,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4371,7 +4372,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4532,7 +4533,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5545,7 +5546,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		 * null, but since it's filled specially, there's no need to test
 		 * anything.)
 		 */
-		tab->new_notnull |= colDef->is_not_null;
+		tab->verify_new_notnull |= colDef->is_not_null;
 	}
 
 	/*
@@ -5948,8 +5949,17 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		/*
+		 * Verifying table data can be done in Phase 3 with single table scan
+		 * for all constraint (both existing and new)
+		 * We can skip this check only if every new 

Re: using index or check in ALTER TABLE SET NOT NULL

[Ildar Musin]

On 06.03.2018 16:12, Sergei Kornilov wrote:

Hello thank you for review!


Adding check constraint will also force the full table scan. So I
think it would be better to rephrased it as follows:

Agree. I updated docs in new attached patch slightly different


Regarding regression tests it may be useful to set
client_min_messages to 'debug1' before setting "NOT NULL" attribute
for a column. In this case you can tell for sure that
NotNullImpliedByRelConstraints() returned true (i.e. your code
actually did the job) as the special debug message is printed to
the log.

I can not find any debug messages in tests: grep client_min_messages
-irn src/test/ Only some warning level and few error. So i verify in
regression tests only top-level behavior. Or this paragraph was for
other people, not for tests?



Sorry, probably I didn't explain it clearly enough. I meant that your
regression tests can't distinguish cases when the full table scan was
actually performed from the ones when it was skipped due to
NotNullImpliedByRelConstraints() check. For instance, consider this
piece from the test suite:


# create table atacc1 (test_a int, test_b int);
CREATE TABLE
...

# alter table atacc1 add constraint atacc1_constr_a_valid check(test_a
is not null);
ALTER TABLE

# alter table atacc1 alter test_a set not null;
ALTER TABLE


It is not obvious that full table scan was omitted. But if we set
client_min_messages to 'debug1', we'll be able to see what is really
happened by the different debug messages. For example:


# create table atacc1 (test_a int, test_b int);
CREATE TABLE
...

# set client_min_messages to 'debug1';
SET

# alter table atacc1 alter test_a set not null;
DEBUG:  verifying table "atacc1"    full table scan!
ALTER TABLE

# alter table atacc1 alter test_a drop not null;
ALTER TABLE

# alter table atacc1 add constraint atacc1_constr_a_valid check(test_a
is not null);
DEBUG:  verifying table "atacc1"
ALTER TABLE

# alter table atacc1 alter test_a set not null;
DEBUG:  verifying table "atacc1" NOT NULL constraint on test_a attribute
by existed constraints    full scan was skipped!
ALTER TABLE


--
Ildar Musin
i.mu...@postgrespro.ru

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello
thank you for review!

> Adding check constraint will also force the full table scan. So I think
> it would be better to rephrased it as follows:
Agree. I updated docs in new attached patch slightly different

> Regarding regression tests it may be useful to set client_min_messages
> to 'debug1' before setting "NOT NULL" attribute for a column. In this
> case you can tell for sure that NotNullImpliedByRelConstraints()
> returned true (i.e. your code actually did the job) as the special debug
> message is printed to the log.
I can not find any debug messages in tests: grep client_min_messages -irn 
src/test/ Only some warning level and few error. So i verify in regression 
tests only top-level behavior.
Or this paragraph was for other people, not for tests?

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index afe2139..783a0ef 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -215,8 +215,15 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  You can only use SET NOT NULL when the column 
+  contains no null values. Full table scan is performed to check 
+  this condition unless there are valid CHECK 
+  constraints that prohibit NULL values for specified column.
+  In the latter case table scan is skipped.
  
 
  
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index fcf7655..4fec36d 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1258,7 +1258,7 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 	 * not contain any row that would belong to the new partition, we can
 	 * avoid scanning the default partition.
 	 */
-	if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
+	if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
 	{
 		ereport(INFO,
 (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
@@ -1302,8 +1302,8 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 			 * that it will not contain any row that would belong to the new
 			 * partition, we can avoid scanning the child table.
 			 */
-			if (PartConstraintImpliedByRelConstraint(part_rel,
-	 def_part_constraints))
+			if (ConstraintImpliedByRelConstraint(part_rel,
+ def_part_constraints))
 			{
 ereport(INFO,
 		(errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 74e020b..3f7218d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -162,7 +162,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -377,6 +377,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4371,7 +4372,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4532,7 +4533,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5545,7 +5546,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		 * null, but since it's filled specially, there's no need to test
 

Re: using index or check in ALTER TABLE SET NOT NULL

[Ildar Musin]

Hello Sergei,

I couldn't find any case when your code doesn't work properly. So it 
seems ok to me.



@@ -220,6 +220,13 @@ WITH ( MODULUS numeric_literal, REM
  

  
+  Full table scan is performed to check that no existing row
+  in the table has null values in given column.  It is possible to avoid
+  this scan by adding a valid CHECK constraint to
+  the table that would allow only NOT NULL values for given column.
+ 


Adding check constraint will also force the full table scan. So I think 
it would be better to rephrased it as follows:


"Full table scan is performed to check that column doesn't contain NULL 
values unless there are valid check constraints that prohibit NULL 
values for specified column. In the latter case table scan is skipped."


A native English speaker input would be helpful here.

Regarding regression tests it may be useful to set client_min_messages 
to 'debug1' before setting "NOT NULL" attribute for a column. In this 
case you can tell for sure that NotNullImpliedByRelConstraints() 
returned true (i.e. your code actually did the job) as the special debug 
message is printed to the log.


Thanks!

--
Ildar Musin
i.mu...@postgrespro.ru

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello Stephen

I changed the suggested things and added some regression tests. Also i wrote 
few words to the documentation. New patch is attached.

Thank you for feedback!
regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 286c7a8..cf2c56f 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -189,6 +189,13 @@ ALTER TABLE [ IF EXISTS ] name
  
 
  
+  Full table scan is performed to check that no existing row
+  in the table has null values in given column.  It is possible to avoid
+  this scan by adding a valid CHECK constraint to
+  the table that would allow only NOT NULL values for given column.
+ 
+
+ 
   If this table is a partition, one cannot perform DROP NOT NULL
   on a column if it is marked NOT NULL in the parent
   table.  To drop the NOT NULL constraint from all the
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 8adc4ee..6f7ec4a 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1235,7 +1235,7 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 	 * not contain any row that would belong to the new partition, we can
 	 * avoid scanning the default partition.
 	 */
-	if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
+	if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
 	{
 		ereport(INFO,
 (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
@@ -1279,8 +1279,8 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 			 * that it will not contain any row that would belong to the new
 			 * partition, we can avoid scanning the child table.
 			 */
-			if (PartConstraintImpliedByRelConstraint(part_rel,
-	 def_part_constraints))
+			if (ConstraintImpliedByRelConstraint(part_rel,
+ def_part_constraints))
 			{
 ereport(INFO,
 		(errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2e768dd..4f0916d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -162,7 +162,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -377,6 +377,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4368,7 +4369,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4529,7 +4530,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5528,7 +5529,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		 * null, but since it's filled specially, there's no need to test
 		 * anything.)
 		 */
-		tab->new_notnull |= colDef->is_not_null;
+		tab->verify_new_notnull |= colDef->is_not_null;
 	}
 
 	/*
@@ -5930,8 +5931,17 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		/*
+		 * Verifying table data can be done in Phase 3 with single table scan
+		 * for all constraint (both existing and new)
+		 * We can skip this check only if every new NOT NULL contraint
+		 * implied by existed table constraints
+		 */
+		if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+		{
+			

Re: using index or check in ALTER TABLE SET NOT NULL

[Stephen Frost]

Greetings Sergei,

* Sergei Kornilov (s...@zsrv.org) wrote:
> I update patch and also rebase to current head. I hope now it is better 
> aligned with project style

Thanks for updating it and continuing to work on it.  I haven't done a
full review but there were a few things below that I thought could be
improved-

> @@ -5863,8 +5864,11 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
>  
>   CatalogTupleUpdate(attr_rel, >t_self, tuple);
>  
> - /* Tell Phase 3 it needs to test the constraint */
> - tab->new_notnull = true;
> + if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) 
> GETSTRUCT(tuple)))
> + {
> + /* Tell Phase 3 it needs to test the constraint */
> + tab->verify_new_notnull = true;
> + }
>  
>   ObjectAddressSubSet(address, RelationRelationId,
>   RelationGetRelid(rel), 
> attnum);

This could really use some additional comments, imv.  In particular, we
need to make it clear that verify_new_notnull only moves from the
initial 'false' value to 'true', since we could be asked to add multiple
NOT NULL constraints and if any of them aren't already covered by an
existing CHECK constraint then we need to perform the full table check.

> @@ -13618,15 +13662,14 @@ ComputePartitionAttrs(Relation rel, List 
> *partParams, AttrNumber *partattrs,
>  }
>  
>  /*
> - * PartConstraintImpliedByRelConstraint
> - *   Does scanrel's existing constraints imply the partition 
> constraint?
> + * ConstraintImpliedByRelConstraint
> + *   Does scanrel's existing constraints imply given constraint
>   *
>   * Existing constraints includes its check constraints and column-level
>   * NOT NULL constraints and partConstraint describes the partition 
> constraint.
>   */
>  bool
> -PartConstraintImpliedByRelConstraint(Relation scanrel,
> -  List 
> *partConstraint)
> +ConstraintImpliedByRelConstraint(Relation scanrel, List *partConstraint)
>  {
>   List   *existConstraint = NIL;
>   TupleConstr *constr = RelationGetDescr(scanrel)->constr;

I would also rename 'partConstraint' since this function is no longer,
necessairly, working with a partition's constraint.

This could also really use some regression tests and I'd be sure to
include tests of adding multiple NOT NULL constraints, sometimes where
they're all covered by existing CHECK constraints and other times when
only one or the other is (and there's existing NULL values in the other
column), etc.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello
I update patch and also rebase to current head. I hope now it is better aligned 
with project stylediff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index dd4a8d3..36bcb3f 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1227,7 +1227,7 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 	 * not contain any row that would belong to the new partition, we can
 	 * avoid scanning the default partition.
 	 */
-	if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
+	if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
 	{
 		ereport(INFO,
 (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
@@ -1271,8 +1271,8 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 			 * that it will not contain any row that would belong to the new
 			 * partition, we can avoid scanning the child table.
 			 */
-			if (PartConstraintImpliedByRelConstraint(part_rel,
-	 def_part_constraints))
+			if (ConstraintImpliedByRelConstraint(part_rel,
+ def_part_constraints))
 			{
 ereport(INFO,
 		(errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d979ce2..35eac39 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -162,7 +162,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -370,6 +370,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4304,7 +4305,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4465,7 +4466,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5461,7 +5462,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		 * null, but since it's filled specially, there's no need to test
 		 * anything.)
 		 */
-		tab->new_notnull |= colDef->is_not_null;
+		tab->verify_new_notnull |= colDef->is_not_null;
 	}
 
 	/*
@@ -5863,8 +5864,11 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+		{
+			/* Tell Phase 3 it needs to test the constraint */
+			tab->verify_new_notnull = true;
+		}
 
 		ObjectAddressSubSet(address, RelationRelationId,
 			RelationGetRelid(rel), attnum);
@@ -5881,6 +5885,46 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 }
 
 /*
+ * NotNullImpliedByRelConstraints
+ *		Does rel's existing constraints imply NOT NULL for given attribute
+ */
+static bool
+NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr)
+{
+	List	   *notNullConstraint = NIL;
+	NullTest   *nnulltest = makeNode(NullTest);
+
+	nnulltest->arg = (Expr *) makeVar(1,
+	  attr->attnum,
+	  attr->atttypid,
+	  attr->atttypmod,
+	  attr->attcollation,
+	  0);
+	nnulltest->nulltesttype = IS_NOT_NULL;
+
+	/*
+	 * same thing as in ConstraintImpliedByRelConstraint argisrow=false is
+	 * correct even for a composite column, because attnotnull does not
+	 * represent a SQL-spec IS NOT NULL test in such a case, just IS DISTINCT
+	 * FROM NULL.
+	 */
+	nnulltest->argisrow = false;
+	

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Thank you for review! My apologies for my errors. It seems i read developers 
wiki pages not enough carefully. I will reread wiki, code style and then update 
patch with all your remarks.

> The comment /* T if we added new NOT NULL constraints */ should
> probably be changed to /* T if we should recheck NOT NULL constraints
> */ or similar.
Correct. Hm, probably I will also rename this property to verify_new_notnull 
for clarity

> I'd suggest registering this with the next CommitFest.
Already done in https://commitfest.postgresql.org/16/1389/ , i noticed this 
step in wiki

Re: using index or check in ALTER TABLE SET NOT NULL

[Andres Freund]

On November 29, 2017 8:50:31 AM PST, Stephen Frost  wrote:
>As for conflicting snapshots, isn't the lock we're taking already
>AccessExclusive..?

Doesn't help if e.g. the current xact is repeatable read or if your own xact 
deleted things (other xacts with snapshots could still see null rows, despite 
new take definition). 

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Here is new patch with check only existed valid constraints and without SPI at 
all.

Thanksdiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d979ce2..7ab7580 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -370,6 +370,8 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool isSetNotNullNeedsTableScan(Relation rel, Form_pg_attribute attr);
+static bool ConstraintImpliedByRelConstraint(Relation scanrel, List *partConstraint);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -5863,8 +5865,10 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		if (isSetNotNullNeedsTableScan(rel, (Form_pg_attribute) GETSTRUCT(tuple))) {
+			/* Tell Phase 3 it needs to test the constraint */
+			tab->new_notnull = true;
+		}
 
 		ObjectAddressSubSet(address, RelationRelationId,
 			RelationGetRelid(rel), attnum);
@@ -5880,6 +5884,42 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 	return address;
 }
 
+static bool
+isSetNotNullNeedsTableScan(Relation rel, Form_pg_attribute attr)
+{
+	List   *notNullConstraint = NIL;
+	NullTest   *nnulltest = makeNode(NullTest);
+
+	nnulltest->arg = (Expr *) makeVar(1,
+  attr->attnum,
+  attr->atttypid,
+  attr->atttypmod,
+  attr->attcollation,
+  0);
+	nnulltest->nulltesttype = IS_NOT_NULL;
+
+	/*
+	 * same thing as in ConstraintImpliedByRelConstraint
+	 * argisrow=false is correct even for a composite column,
+	 * because attnotnull does not represent a SQL-spec IS NOT
+	 * NULL test in such a case, just IS DISTINCT FROM NULL.
+	 */
+	nnulltest->argisrow = false;
+	nnulltest->location = -1;
+	notNullConstraint = lappend(notNullConstraint, nnulltest);
+
+	if (ConstraintImpliedByRelConstraint(rel, notNullConstraint))
+	{
+		ereport(DEBUG1,
+			(errmsg("verifying table \"%s\" NOT NULL constraint "
+"on %s attribute by existed constraints",
+			RelationGetRelationName(rel), NameStr(attr->attname;
+		return false;
+	}
+
+	return true;
+}
+
 /*
  * ALTER TABLE ALTER COLUMN SET/DROP DEFAULT
  *
@@ -13620,12 +13660,23 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
 /*
  * PartConstraintImpliedByRelConstraint
  *		Does scanrel's existing constraints imply the partition constraint?
+ */
+bool
+PartConstraintImpliedByRelConstraint(Relation scanrel,
+	 List *partConstraint)
+{
+	return ConstraintImpliedByRelConstraint(scanrel, partConstraint);
+}
+
+/*
+ * PartConstraintImpliedByRelConstraint
+ *		Does scanrel's existing constraints imply given constraint
  *
  * Existing constraints includes its check constraints and column-level
  * NOT NULL constraints and partConstraint describes the partition constraint.
  */
-bool
-PartConstraintImpliedByRelConstraint(Relation scanrel,
+static bool
+ConstraintImpliedByRelConstraint(Relation scanrel,
 	 List *partConstraint)
 {
 	List	   *existConstraint = NIL;

Re: using index or check in ALTER TABLE SET NOT NULL

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> I did not look at the patch yet, but TBH if it uses SPI for sub-operations
>> of ALTER TABLE I think that is sufficient reason to reject it out of hand.

> You mean like what ALTER TABLE ... ADD FOREIGN KEY does?

Yeah, and if you look at the warts that SPI has grown to support that
usage, you'll see why I'm so unhappy.  We should never have allowed
FKs to be built on top of SPI; they require semantics that don't exist
in SQL.  I think this would lead to more of the same --- not exactly
the same of course, but more warts.  See Robert's nearby musings about
semantics of index null checks for an example.

regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

[Robert Haas]

On Wed, Nov 29, 2017 at 10:52 AM, Sergei Kornilov  wrote:
> I agree this. Thinking a little about idea of index scan i can not give 
> reasonable usecase which required index. My target problem of adding NOT NULL 
> to big relation without long downtime can be done with ADD CONSTRAINT NOT 
> VALID, VALIDATE it in second transaction, then SET NOT NULL by my patch and 
> drop unneeded constraint.

Yes, I had the same thought.

Thinking a little bit further, maybe the idea you had in mind using
the index scan was that some indexes offer cheap ways of testing
whether ANY nulls are present in the column.  For example, if we had a
non-partial btree index whose first column is the one being made NOT
NULL, we could try an index scan - via index_beginscan() /
index_getnext() / index_endscan() - trying to pull exactly one null
from the index, and judge whether or not there are nulls present based
only whether we get one.  This would be a lot cheaper than scanning a
large table, but it needs some careful thought because of visibility
issues.  It's not sufficient that the index contains no nulls that are
visible to our snapshot; it must contain no nulls that are visible to
any plausible current or future snapshot.  I doubt that test can be
written in SQL, but it can probably be written in C.  Moreover, we
need to avoid not only false negatives (thinking that there is no NULL
when there is one) but also false positives (thinking there's a NULL
in the column when there isn't, and thus failing spuriously).  But it
seems like it might be useful if someone can figure out the details of
how to make it 100% correct; one index lookup is sure to be a lot
quicker than a full table scan.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Isn't the first concern addressed by using SPI..?
> 
> I did not look at the patch yet, but TBH if it uses SPI for sub-operations
> of ALTER TABLE I think that is sufficient reason to reject it out of hand.

You mean like what ALTER TABLE ... ADD FOREIGN KEY does?

> Doing things that way would create way too much of a vulnerability surface
> for code touching a partially-updated table.  At minimum, we'd have to
> blow holes in existing protections like CheckTableNotInUse, and I think
> we'd be forever finding other stuff that failed to work quite right in
> that context.  I do not want ALTER TABLE going anywhere near the planner
> or executor; I'm not even happy that it uses the parser (for index
> definition reconstruction).

That's more along the lines of the kind of response I was expecting
given the suggestion, and perhaps a good reason to just go with the
index-based lookup, when an index is available to do so with, but I'm
not entirely sure how this is different from how we handle foreign keys.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: using index or check in ALTER TABLE SET NOT NULL

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Nov 28, 2017 at 1:59 PM, Sergei Kornilov  wrote:
> > I write patch to speed up ALTER TABLE SET NOT NULL by check existed check 
> > constraints or indexes. Huge phase 3 with verify table data will be skipped 
> > if table has valid check constraint cover "alteredfield IS NOT NULL" 
> > condition or by SPI query if found index with compatible condition or 
> > regular amsearchnulls index on processed field.
> 
> Doing this based on the existence of a valid constraint which implies
> that no nulls can be present seems like a good idea.  Doing it based
> on an index scan doesn't necessarily seem like a good idea.  We have
> no guarantee at all that the index scan will be faster than scanning
> the table would have been, and a single table scan can do multiple
> verification steps if, for example, multiple columns are set NOT NULL
> at the same time.

Isn't the first concern addressed by using SPI..?

As for the second concern, couldn't that be done with a more complicated
query through SPI, though things might have to be restructured some to
make it possible to do that.

Just, generally speaking, this is definitely something that I think we
want and neither of the above concerns seem like they're technical
reasons why we can't use something like this approach, just needs to
perhaps be reworked to handle multiple columns in a single query.

Thanks!

Stephen


signature.asc
Description: Digital signature

using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hello
I write patch to speed up ALTER TABLE SET NOT NULL by check existed check 
constraints or indexes. Huge phase 3 with verify table data will be skipped if 
table has valid check constraint cover "alteredfield IS NOT NULL" condition or 
by SPI query if found index with compatible condition or regular amsearchnulls 
index on processed field.

Patch based on current master branch, i believe it has no platform-dependent 
code, of course code compiled and pass tests locally.
Tell me please, what i forgot or make incorrectly.

Implementation notes:
I use existed PartConstraintImpliedByRelConstraint method to check relation 
constraints. But i rename original method to static 
ConstraintImpliedByRelConstraint (because method now used not only in 
partitions) and leave PartConstraintImpliedByRelConstraint as proxy to not 
change public API.
I found it difficult to do index scan and choose index with lower costs if 
found many suitable indexes. Is it acceptable to use SPI here?

Related archive discussions:
https://www.postgresql.org/message-id/flat/530C10CF.4020101%40strangersgate.com
https://www.postgresql.org/message-id/flat/CAASwCXdAK55BzuOy_FtYj2zQWg26PriDKL5pRoWiyFJe0eg-Hg%40mail.gmail.com

Thanks!diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d979ce2..424f608 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -60,6 +60,7 @@
 #include "commands/typecmds.h"
 #include "commands/user.h"
 #include "executor/executor.h"
+#include "executor/spi.h"
 #include "foreign/foreign.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
@@ -370,6 +371,8 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool isSetNotNullNeedTableScan(Relation rel, Form_pg_attribute attr);
+static bool ConstraintImpliedByRelConstraint(Relation scanrel, List *partConstraint);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -5863,8 +5866,10 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, >t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		if (isSetNotNullNeedTableScan(rel, (Form_pg_attribute) GETSTRUCT(tuple))) {
+			/* Tell Phase 3 it needs to test the constraint */
+			tab->new_notnull = true;
+		}
 
 		ObjectAddressSubSet(address, RelationRelationId,
 			RelationGetRelid(rel), attnum);
@@ -5880,6 +5885,148 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 	return address;
 }
 
+static bool
+isSetNotNullNeedTableScan(Relation rel, Form_pg_attribute attr)
+{
+	List   *indexoidlist;
+	ListCell   *indexoidscan;
+	bool   isMatchedIndexFound = false;
+	StringInfoData querybuf;
+	intindexAttr;
+	intcheckIndexAttrNum;
+
+	// same part in PartConstraintImpliedByRelConstraint
+	List   *notNullConstraint = NIL;
+	NullTest   *nnulltest = makeNode(NullTest);
+	List   *nullConstraint = NIL;
+	NullTest   *ntest = makeNode(NullTest);
+
+	nnulltest->arg = (Expr *) makeVar(1,
+  attr->attnum,
+  attr->atttypid,
+  attr->atttypmod,
+  attr->attcollation,
+  0);
+	nnulltest->nulltesttype = IS_NOT_NULL;
+
+	/*
+	 * argisrow=false is correct even for a composite column,
+	 * because attnotnull does not represent a SQL-spec IS NOT
+	 * NULL test in such a case, just IS DISTINCT FROM NULL.
+	 */
+	nnulltest->argisrow = false;
+	nnulltest->location = -1;
+	notNullConstraint = lappend(notNullConstraint, nnulltest);
+
+	// first check existed constraints
+	if (ConstraintImpliedByRelConstraint(rel, notNullConstraint)) {
+		ereport(DEBUG1,
+			(errmsg("verifying table \"%s\" NOT NULL constraint "
+"on %s attribute by existed constraints",
+			RelationGetRelationName(rel), NameStr(attr->attname;
+		return false;
+	}
+
+	ntest->arg = (Expr *) makeVar(1,
+  attr->attnum,
+  attr->atttypid,
+  attr->atttypmod,
+  attr->attcollation,
+  0);
+	ntest->nulltesttype = IS_NULL;
+
+	/*
+	 * argisrow=false is correct even for a composite column,
+	 * because attnotnull does not represent a SQL-spec IS NOT
+	 * NULL test in such a case, just IS DISTINCT FROM NULL.
+	 */
+	ntest->argisrow = false;
+	ntest->location = -1;
+	nullConstraint = lappend(nullConstraint, ntest);
+
+	indexoidlist = RelationGetIndexList(rel);
+	foreach(indexoidscan, indexoidlist)
+	{
+		Oid		 indexoid = lfirst_oid(indexoidscan);
+		Relation	indexRel;
+		Form_pg_index indexStruct;
+
+		indexRel = index_open(indexoid, AccessShareLock);
+		indexStruct = indexRel->rd_index;
+
+		if (IndexIsValid(indexStruct))
+		{
+			if (RelationGetIndexPredicate(indexRel) !=