Re: [HACKERS] New feature "... ALTER CONSTRAINT ... VERIFY USING INDEX"

[Alvaro Herrera]

Vitaly Burovoy wrote:

> I guess you are talking about the other thread[1].
> I'm not sure I have enough experience in Postgres hacking to start
> working on it right now, but I'll have a look.
> IMO the best way is to raise that topic by a letter with summary what
> troubles are left there (in addition to a rebasing one).
> 
> 
> [1] 
> http://www.postgresql.org/message-id/flat/20110707213401.ga27...@alvh.no-ip.org

Here's a newer thread:
https://www.postgresql.org/message-id/1343682669-sup-2532%40alvh.no-ip.org
which includes some points needing additional work.

In my local tree I have a branch that maybe is not the same as the last
patch I posted in that thread, because the last commit has a newer date.
Here I attach what that branch has.  It applies cleanly on top of
03bda4535ee119d3.  I don't remember if it was in compilable state at the
time I abandoned it.

It needs some work to rebase, but from a quick experimental "git merge"
I just ran, it's not too bad.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/constraint.c b/src/backend/commands/constraint.c
index 8ac8373..4c9686f 100644
--- a/src/backend/commands/constraint.c
+++ b/src/backend/commands/constraint.c
@@ -14,12 +14,16 @@
 #include "postgres.h"
 
 #include "catalog/index.h"
+#include "catalog/pg_constraint.h"
+#include "commands/constraint.h"
 #include "commands/trigger.h"
 #include "executor/executor.h"
 #include "utils/builtins.h"
 #include "utils/rel.h"
+#include "utils/syscache.h"
 #include "utils/tqual.h"
 
+static char *tryExtractNotNull_internal(Node *node, Relation rel);
 
 /*
  * unique_key_recheck - trigger function to do a deferred uniqueness check.
@@ -188,3 +192,121 @@ unique_key_recheck(PG_FUNCTION_ARGS)
 
 	return PointerGetDatum(NULL);
 }
+
+Constraint *
+createCheckNotNullConstraint(Oid nspid, char *constraint_name,
+			 const char *relname, const char *colname)
+{
+	Constraint *check = makeNode(Constraint);
+	ColumnRef  *colref;
+	NullTest   *nulltest;
+
+	colref = (ColumnRef *) makeNode(ColumnRef);
+	colref->fields = list_make1(makeString(pstrdup(colname)));
+
+	nulltest = (NullTest *) makeNode(NullTest);
+	nulltest->argisrow = false;	/* FIXME -- may be bogus! */
+	nulltest->nulltesttype = IS_NOT_NULL;
+	nulltest->arg = (Expr *) colref;
+
+	check->contype = CONSTR_CHECK;
+	check->location = -1;
+	check->conname = constraint_name ? constraint_name :
+		ChooseConstraintName(relname, colname, "not_null", nspid,
+			 NIL);
+	check->raw_expr = (Node *) nulltest;
+	check->cooked_expr = NULL;
+
+	return check;
+}
+
+/*
+ * Given a CHECK constraint, examine it and determine whether it is CHECK (col
+ * IS NOT NULL).  If it is, return the column name for which it is.  Otherwise
+ * return NULL.
+ */
+char *
+tryExtractNotNullFromCheckConstr(Constraint *constr)
+{
+	char   *retval;
+
+	Assert(constr->contype == CONSTR_CHECK);
+
+	if (constr->raw_expr != NULL)
+	{
+		retval = tryExtractNotNull_internal(constr->raw_expr, NULL);
+		if (retval != NULL)
+			return retval;
+	}
+
+	return tryExtractNotNull_internal(stringToNode(constr->cooked_expr), NULL);
+}
+
+/*
+ * As above, but use a pg_constraint row as input.
+ *
+ * tupdesc is pg_constraint's tuple descriptor, and rel is the relation the
+ * constraint is for.
+ */
+char *
+tryExtractNotNullFromCatalog(HeapTuple constrTup, TupleDesc tupdesc,
+			 Relation rel)
+{
+	Datum	val;
+	bool	isnull;
+	char   *conbin;
+	Node   *node;
+
+	val = SysCacheGetAttr(CONSTROID, constrTup, Anum_pg_constraint_conbin,
+		  &isnull);
+	if (isnull)
+		elog(ERROR, "null conbin for constraint %u",
+			 HeapTupleGetOid(constrTup));
+	conbin = TextDatumGetCString(val);
+	node = (Node *) stringToNode(conbin);
+
+	return tryExtractNotNull_internal(node, rel);
+}
+
+/*
+ * Worker for the above
+ */
+static char *
+tryExtractNotNull_internal(Node *node, Relation rel)
+{
+	if (IsA(node, NullTest))
+	{
+		NullTest *nulltest = (NullTest *) node;
+
+		if (nulltest->nulltesttype == IS_NOT_NULL)
+		{
+			if (IsA(nulltest->arg, ColumnRef))
+			{
+ColumnRef *colref = (ColumnRef *) nulltest->arg;
+
+if (list_length(colref->fields) == 1)
+	return strVal(linitial(colref->fields));
+			}
+			if (IsA(nulltest->arg, Var))
+			{
+Var	   *var = (Var *) nulltest->arg;
+TupleDesc tupdesc;
+
+if (!RelationIsValid(rel))
+	elog(ERROR,
+		 "no relation given to extract constraint from");
+tupdesc = RelationGetDescr(rel);
+return NameStr(tupdesc->attrs[var->varattno - 1]->attname);
+			}
+		}
+	}
+
+	/*
+	 * XXX Need to check a few more possible wordings of NOT NULL:
+	 *
+	 * - foo IS DISTINCT FROM NULL
+	 * - NOT (foo IS NULL)
+	 */
+
+	return NULL;
+}
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index dc0665e..a3bff7d 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -231,6 +231,7 @@ intore

Re: [HACKERS] New feature "... ALTER CONSTRAINT ... VERIFY USING INDEX"

[Vitaly Burovoy]

On 1/8/16, Alvaro Herrera  wrote:
> Simon Riggs wrote:
>> On 8 January 2016 at 14:14, Vitaly Burovoy 
>> wrote:
>>
>> > What about SET NOT NULL constraints? There is no VALIDATE CONSTRAINT
>> > for
>> > it.
>>
>> Sounds like a useful addition.
>
> Yes.  In order to make it a reality you need to make the NOT NULL
> constraints appear in pg_constraint.  Years ago I wrote a patch to do
> that, which was very close to done.  It would be really cool if Vitaly
> or someone else could look into that patch, update it and get it ready
> for commit.
>
> If someone is interested, I can send the patch along.
>
> --
> Álvaro Herrerahttp://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

I guess you are talking about the other thread[1].
I'm not sure I have enough experience in Postgres hacking to start
working on it right now, but I'll have a look.
IMO the best way is to raise that topic by a letter with summary what
troubles are left there (in addition to a rebasing one).


[1] 
http://www.postgresql.org/message-id/flat/20110707213401.ga27...@alvh.no-ip.org

-- 
Best regards,
Vitaly Burovoy


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New feature "... ALTER CONSTRAINT ... VERIFY USING INDEX"

[Alvaro Herrera]

Simon Riggs wrote:
> On 8 January 2016 at 14:14, Vitaly Burovoy  wrote:
> 
> > What about SET NOT NULL constraints? There is no VALIDATE CONSTRAINT for
> > it.
> 
> Sounds like a useful addition.

Yes.  In order to make it a reality you need to make the NOT NULL
constraints appear in pg_constraint.  Years ago I wrote a patch to do
that, which was very close to done.  It would be really cool if Vitaly
or someone else could look into that patch, update it and get it ready
for commit.

If someone is interested, I can send the patch along.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New feature "... ALTER CONSTRAINT ... VERIFY USING INDEX"

[Simon Riggs]

On 8 January 2016 at 14:14, Vitaly Burovoy  wrote:

>
> What about SET NOT NULL constraints? There is no VALIDATE CONSTRAINT for
> it.


Sounds like a useful addition.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] New feature "... ALTER CONSTRAINT ... VERIFY USING INDEX"

[Vitaly Burovoy]

On 1/8/16, Simon Riggs  wrote:
> On 8 January 2016 at 13:13, Vitaly Burovoy 
> wrote:
>
>> On 1/8/16, Simon Riggs  wrote:
>> > On 8 January 2016 at 12:49, Vitaly Burovoy 
>> > wrote:
>> >
>> >
>> >> In Postgres9.1 a new feature was implemented [1] for adding PK and
>> >> UNIQUE constraints using indexes created concurrently, but constraints
>> >> NOT NULL and CHECK still require full seqscan of a table. New CHECK
>> >> constraint allows "NOT VALID" option but VALIDATE CONSTRAINT still
>> >> does seqscan (with RowExclusiveLock, but for big and constantly
>> >> updatable table it is still awful).
>> >>
>> >> It is possible to find wrong rows in a table without seqscan if there
>> >> is an index with a predicate allows to find such rows. There is no
>> >> sense what columns it has since it is enough to check whether
>> >> index_getnext for it returns NULL (table is OK) or any tuple (table
>> >> has wrong rows).
>> >>
>> >
>> > You avoid a full seqscan by creating an index which also does a full
>> > seq
>> > scan.
>> >
>> > How does this help? The lock and scan times are the same.
>>
>> I avoid not a full seqscan, but a time when table is under
>> ExclusiveLock: index can be build concurrently without locking table.
>
>
> That is exactly what ADD ...NOT VALID  and VALIDATE already does, as of
> 9.4.
>
> --
> Simon Riggshttp://www.2ndQuadrant.com/
> 
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Hmm... It really does. I was confused by a line in ATExecValidateConstraint
conrel = heap_open(ConstraintRelationId, RowExclusiveLock);

but validateCheckConstraint doesn't do anything for applying the lock to a row.

What about SET NOT NULL constraints? There is no VALIDATE CONSTRAINT for it.
-- 
Best regards,
Vitaly Burovoy


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New feature "... ALTER CONSTRAINT ... VERIFY USING INDEX"

[Simon Riggs]

On 8 January 2016 at 13:13, Vitaly Burovoy  wrote:

> On 1/8/16, Simon Riggs  wrote:
> > On 8 January 2016 at 12:49, Vitaly Burovoy 
> > wrote:
> >
> >
> >> In Postgres9.1 a new feature was implemented [1] for adding PK and
> >> UNIQUE constraints using indexes created concurrently, but constraints
> >> NOT NULL and CHECK still require full seqscan of a table. New CHECK
> >> constraint allows "NOT VALID" option but VALIDATE CONSTRAINT still
> >> does seqscan (with RowExclusiveLock, but for big and constantly
> >> updatable table it is still awful).
> >>
> >> It is possible to find wrong rows in a table without seqscan if there
> >> is an index with a predicate allows to find such rows. There is no
> >> sense what columns it has since it is enough to check whether
> >> index_getnext for it returns NULL (table is OK) or any tuple (table
> >> has wrong rows).
> >>
> >
> > You avoid a full seqscan by creating an index which also does a full seq
> > scan.
> >
> > How does this help? The lock and scan times are the same.
>
> I avoid not a full seqscan, but a time when table is under
> ExclusiveLock: index can be build concurrently without locking table.


That is exactly what ADD ...NOT VALID  and VALIDATE already does, as of 9.4.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] New feature "... ALTER CONSTRAINT ... VERIFY USING INDEX"

[Vitaly Burovoy]

On 1/8/16, Simon Riggs  wrote:
> On 8 January 2016 at 12:49, Vitaly Burovoy 
> wrote:
>
>
>> In Postgres9.1 a new feature was implemented [1] for adding PK and
>> UNIQUE constraints using indexes created concurrently, but constraints
>> NOT NULL and CHECK still require full seqscan of a table. New CHECK
>> constraint allows "NOT VALID" option but VALIDATE CONSTRAINT still
>> does seqscan (with RowExclusiveLock, but for big and constantly
>> updatable table it is still awful).
>>
>> It is possible to find wrong rows in a table without seqscan if there
>> is an index with a predicate allows to find such rows. There is no
>> sense what columns it has since it is enough to check whether
>> index_getnext for it returns NULL (table is OK) or any tuple (table
>> has wrong rows).
>>
>
> You avoid a full seqscan by creating an index which also does a full seq
> scan.
>
> How does this help? The lock and scan times are the same.
>
> --
> Simon Riggshttp://www.2ndQuadrant.com/
> 
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

I avoid not a full seqscan, but a time when table is under
ExclusiveLock: index can be build concurrently without locking table.

-- 
Best regards,
Vitaly Burovoy


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New feature "... ALTER CONSTRAINT ... VERIFY USING INDEX"

[Simon Riggs]

On 8 January 2016 at 12:49, Vitaly Burovoy  wrote:


> In Postgres9.1 a new feature was implemented [1] for adding PK and
> UNIQUE constraints using indexes created concurrently, but constraints
> NOT NULL and CHECK still require full seqscan of a table. New CHECK
> constraint allows "NOT VALID" option but VALIDATE CONSTRAINT still
> does seqscan (with RowExclusiveLock, but for big and constantly
> updatable table it is still awful).
>
> It is possible to find wrong rows in a table without seqscan if there
> is an index with a predicate allows to find such rows. There is no
> sense what columns it has since it is enough to check whether
> index_getnext for it returns NULL (table is OK) or any tuple (table
> has wrong rows).
>

You avoid a full seqscan by creating an index which also does a full seq
scan.

How does this help? The lock and scan times are the same.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services