[PATCH 2/8] Move IS [NOT] NULL handling from BRIN support functions

public inbox for [email protected]  
help / color / mirror / Atom feed

[PATCH 2/8] Move IS [NOT] NULL handling from BRIN support functions
5+ messages / 4 participants
[nested] [flat]

* [PATCH 2/8] Move IS [NOT] NULL handling from BRIN support functions
@ 2020-09-17 15:26 Tomas Vondra <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Tomas Vondra @ 2020-09-17 15:26 UTC (permalink / raw)

The handling of IS [NOT] NULL clauses is independent of an opclass, and
most of the code was exactly the same in both minmax and inclusion. So
instead move the code from support procedures to the AM methods etc.

This simplifies the code quite a bit - especially the support procedures
quite a bit, as they don't need to care about NULL values and flags at
all. It also means the IS [NOT] NULL clauses can be evaluated without
invoking the support procedure at all.

Author: Tomas Vondra <[email protected]>
Author: Nikita Glukhov <[email protected]>
Reviewed-by: Alvaro Herrera <[email protected]>
Reviewed-by: Mark Dilger <[email protected]>
Reviewed-by: Alexander Korotkov <[email protected]>
Reviewed-by: Masahiko Sawada <[email protected]>
Reviewed-by: John Naylor <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
 src/backend/access/brin/brin.c           | 293 +++++++++++++++++------
 src/backend/access/brin/brin_inclusion.c | 106 +-------
 src/backend/access/brin/brin_minmax.c    | 103 +-------
 src/include/access/brin_internal.h       |   3 +
 4 files changed, 239 insertions(+), 266 deletions(-)

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index dc187153aa..14da9ed17f 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -35,6 +35,7 @@
 #include "storage/freespace.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
+#include "utils/datum.h"
 #include "utils/index_selfuncs.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
@@ -77,7 +78,9 @@ static void form_and_insert_tuple(BrinBuildState *state);
 static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a,
 						 BrinTuple *b);
 static void brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy);
-
+static bool add_values_to_range(Relation idxRel, BrinDesc *bdesc,
+					BrinMemTuple *dtup, Datum *values, bool *nulls);
+static bool check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys);
 
 /*
  * BRIN handler function: return IndexAmRoutine with access method parameters
@@ -179,7 +182,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
 		OffsetNumber off;
 		BrinTuple  *brtup;
 		BrinMemTuple *dtup;
-		int			keyno;
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -243,31 +245,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
 
 		dtup = brin_deform_tuple(bdesc, brtup, NULL);
 
-		/*
-		 * Compare the key values of the new tuple to the stored index values;
-		 * our deformed tuple will get updated if the new tuple doesn't fit
-		 * the original range (note this means we can't break out of the loop
-		 * early). Make a note of whether this happens, so that we know to
-		 * insert the modified tuple later.
-		 */
-		for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
-		{
-			Datum		result;
-			BrinValues *bval;
-			FmgrInfo   *addValue;
-
-			bval = &dtup->bt_columns[keyno];
-			addValue = index_getprocinfo(idxRel, keyno + 1,
-										 BRIN_PROCNUM_ADDVALUE);
-			result = FunctionCall4Coll(addValue,
-									   idxRel->rd_indcollation[keyno],
-									   PointerGetDatum(bdesc),
-									   PointerGetDatum(bval),
-									   values[keyno],
-									   nulls[keyno]);
-			/* if that returned true, we need to insert the updated tuple */
-			need_insert |= DatumGetBool(result);
-		}
+		need_insert = add_values_to_range(idxRel, bdesc, dtup, values, nulls);
 
 		if (!need_insert)
 		{
@@ -390,8 +368,10 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 	BrinMemTuple *dtup;
 	BrinTuple  *btup = NULL;
 	Size		btupsz = 0;
-	ScanKey	  **keys;
-	int		   *nkeys;
+	ScanKey	  **keys,
+			  **nullkeys;
+	int		   *nkeys,
+			   *nnullkeys;
 	int			keyno;
 
 	opaque = (BrinOpaque *) scan->opaque;
@@ -416,10 +396,13 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 
 	/*
 	 * Make room for per-attribute lists of scan keys that we'll pass to the
-	 * consistent support procedure.
+	 * consistent support procedure. We keep null and regular keys separate,
+	 * so that we can easily pass regular keys to the consistent function.
 	 */
 	keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
+	nullkeys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
 	nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
+	nnullkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
 
 	/*
 	 * Preprocess the scan keys - split them into per-attribute arrays.
@@ -440,23 +423,24 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 				TupleDescAttr(bdesc->bd_tupdesc,
 							  keyattno - 1)->attcollation));
 
-		/* First time we see this index attribute, so init as needed. */
-		if (!keys[keyattno-1])
+		/*
+		 * First time we see this index attribute, so init as needed.
+		 *
+		 * This is a bit of an overkill - we don't know how many scan
+		 * keys are there for a given attribute, so we simply allocate
+		 * the largest number possible (as if all scan keys belonged to
+		 * the same attribute). This may waste a bit of memory, but we
+		 * only expect small number of scan keys in general, so this
+		 * should be negligible, and it's probably cheaper than having
+		 * to repalloc repeatedly.
+		 */
+		if (consistentFn[keyattno - 1].fn_oid == InvalidOid)
 		{
 			FmgrInfo   *tmp;
 
-			/*
-			 * This is a bit of an overkill - we don't know how many
-			 * scan keys are there for this attribute, so we simply
-			 * allocate the largest number possible. This may waste
-			 * a bit of memory, but we only expect small number of
-			 * scan keys in general, so this should be negligible,
-			 * and it's cheaper than having to repalloc repeatedly.
-			 */
-			keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
-
-			/* First time this column, so look up consistent function */
-			Assert(consistentFn[keyattno - 1].fn_oid == InvalidOid);
+			/* No key/null arrays for this attribute. */
+			Assert((keys[keyattno - 1] == NULL) && (nkeys[keyattno - 1] == 0));
+			Assert((nullkeys[keyattno - 1] == NULL) && (nnullkeys[keyattno - 1] == 0));
 
 			tmp = index_getprocinfo(idxRel, keyattno,
 									BRIN_PROCNUM_CONSISTENT);
@@ -464,9 +448,23 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 						   CurrentMemoryContext);
 		}
 
-		/* Add key to the per-attribute array. */
-		keys[keyattno - 1][nkeys[keyattno - 1]] = key;
-		nkeys[keyattno - 1]++;
+		/* Add key to the proper per-attribute array. */
+		if (key->sk_flags & SK_ISNULL)
+		{
+			if (!nullkeys[keyattno - 1])
+				nullkeys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
+
+			nullkeys[keyattno - 1][nnullkeys[keyattno - 1]] = key;
+			nnullkeys[keyattno - 1]++;
+		}
+		else
+		{
+			if (!keys[keyattno - 1])
+				keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
+
+			keys[keyattno - 1][nkeys[keyattno - 1]] = key;
+			nkeys[keyattno - 1]++;
+		}
 	}
 
 	/* allocate an initial in-memory tuple, out of the per-range memcxt */
@@ -544,15 +542,57 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 					BrinValues *bval;
 					Datum		add;
 
-					/* skip attributes without any san keys */
-					if (nkeys[attno - 1] == 0)
+					/*
+					 * skip attributes without any scan keys (both regular
+					 * and IS [NOT] NULL)
+					 */
+					if (nkeys[attno - 1] == 0 && nnullkeys[attno - 1] == 0)
 						continue;
 
 					bval = &dtup->bt_columns[attno - 1];
 
+					/*
+					 * First check if there are any IS [NOT] NULL scan keys,
+					 * and if we're violating them. In that case we can
+					 * terminate early, without invoking the support function.
+					 *
+					 * As there may be more keys, we can only detemine mismatch
+					 * within this loop.
+					 */
+					if (bdesc->bd_info[attno - 1]->oi_regular_nulls &&
+						!check_null_keys(bval, nullkeys[attno - 1],
+										 nnullkeys[attno - 1]))
+					{
+						/*
+						 * If any of the IS [NOT] NULL keys failed, the page
+						 * range as a whole can't pass. So terminate the loop.
+						 */
+						addrange = false;
+						break;
+					}
+
+					/*
+					 * So either there are no IS [NOT] NULL keys, or all passed.
+					 * If there are no regular scan keys, we're done - the page
+					 * range matches. If there are regular keys, but the page
+					 * range is marked as 'all nulls' it can't possibly pass
+					 * (we're assuming the operators are strict).
+					 */
+
+					/* No regular scan keys - page range as a whole passes. */
+					if (!nkeys[attno - 1])
+						continue;
+
 					Assert((nkeys[attno - 1] > 0) &&
 						   (nkeys[attno - 1] <= scan->numberOfKeys));
 
+					/* If it is all nulls, it cannot possibly be consistent. */
+					if (bval->bv_allnulls)
+					{
+						addrange = false;
+						break;
+					}
+
 					/*
 					 * Check whether the scan key is consistent with the page
 					 * range values; if so, have the pages in the range added
@@ -695,7 +735,6 @@ brinbuildCallback(Relation index,
 {
 	BrinBuildState *state = (BrinBuildState *) brstate;
 	BlockNumber thisblock;
-	int			i;
 
 	thisblock = ItemPointerGetBlockNumber(tid);
 
@@ -724,25 +763,8 @@ brinbuildCallback(Relation index,
 	}
 
 	/* Accumulate the current tuple into the running state */
-	for (i = 0; i < state->bs_bdesc->bd_tupdesc->natts; i++)
-	{
-		FmgrInfo   *addValue;
-		BrinValues *col;
-		Form_pg_attribute attr = TupleDescAttr(state->bs_bdesc->bd_tupdesc, i);
-
-		col = &state->bs_dtuple->bt_columns[i];
-		addValue = index_getprocinfo(index, i + 1,
-									 BRIN_PROCNUM_ADDVALUE);
-
-		/*
-		 * Update dtuple state, if and as necessary.
-		 */
-		FunctionCall4Coll(addValue,
-						  attr->attcollation,
-						  PointerGetDatum(state->bs_bdesc),
-						  PointerGetDatum(col),
-						  values[i], isnull[i]);
-	}
+	(void) add_values_to_range(index, state->bs_bdesc, state->bs_dtuple,
+							   values, isnull);
 }
 
 /*
@@ -1521,6 +1543,39 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
 		FmgrInfo   *unionFn;
 		BrinValues *col_a = &a->bt_columns[keyno];
 		BrinValues *col_b = &db->bt_columns[keyno];
+		BrinOpcInfo *opcinfo = bdesc->bd_info[keyno];
+
+		if (opcinfo->oi_regular_nulls)
+		{
+			/* Adjust "hasnulls". */
+			if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
+				col_a->bv_hasnulls = true;
+
+			/* If there are no values in B, there's nothing left to do. */
+			if (col_b->bv_allnulls)
+				continue;
+
+			/*
+			 * Adjust "allnulls".  If A doesn't have values, just copy the
+			 * values from B into A, and we're done.  We cannot run the
+			 * operators in this case, because values in A might contain
+			 * garbage.  Note we already established that B contains values.
+			 */
+			if (col_a->bv_allnulls)
+			{
+				int			i;
+
+				col_a->bv_allnulls = false;
+
+				for (i = 0; i < opcinfo->oi_nstored; i++)
+					col_a->bv_values[i] =
+						datumCopy(col_b->bv_values[i],
+								  opcinfo->oi_typcache[i]->typbyval,
+								  opcinfo->oi_typcache[i]->typlen);
+
+				continue;
+			}
+		}
 
 		unionFn = index_getprocinfo(bdesc->bd_index, keyno + 1,
 									BRIN_PROCNUM_UNION);
@@ -1574,3 +1629,103 @@ brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy)
 	 */
 	FreeSpaceMapVacuum(idxrel);
 }
+
+static bool
+add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup,
+					Datum *values, bool *nulls)
+{
+	int			keyno;
+	bool		modified = false;
+
+	/*
+	 * Compare the key values of the new tuple to the stored index values;
+	 * our deformed tuple will get updated if the new tuple doesn't fit
+	 * the original range (note this means we can't break out of the loop
+	 * early). Make a note of whether this happens, so that we know to
+	 * insert the modified tuple later.
+	 */
+	for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
+	{
+		Datum		result;
+		BrinValues *bval;
+		FmgrInfo   *addValue;
+
+		bval = &dtup->bt_columns[keyno];
+
+		if (bdesc->bd_info[keyno]->oi_regular_nulls && nulls[keyno])
+		{
+			/*
+			 * If the new value is null, we record that we saw it if it's
+			 * the first one; otherwise, there's nothing to do.
+			 */
+			if (!bval->bv_hasnulls)
+			{
+				bval->bv_hasnulls = true;
+				modified = true;
+			}
+
+			continue;
+		}
+
+		addValue = index_getprocinfo(idxRel, keyno + 1,
+									 BRIN_PROCNUM_ADDVALUE);
+		result = FunctionCall4Coll(addValue,
+								   idxRel->rd_indcollation[keyno],
+								   PointerGetDatum(bdesc),
+								   PointerGetDatum(bval),
+								   values[keyno],
+								   nulls[keyno]);
+		/* if that returned true, we need to insert the updated tuple */
+		modified |= DatumGetBool(result);
+	}
+
+	return modified;
+}
+
+static bool
+check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys)
+{
+	int			keyno;
+
+	/*
+	 * First check if there are any IS [NOT] NULL scan keys, and if we're
+	 * violating them.
+	 */
+	for (keyno = 0; keyno < nnullkeys; keyno++)
+	{
+		ScanKey		key = nullkeys[keyno];
+
+		Assert(key->sk_attno == bval->bv_attno);
+
+		/* Handle only IS NULL/IS NOT NULL tests */
+		if (!(key->sk_flags & SK_ISNULL))
+			continue;
+
+		if (key->sk_flags & SK_SEARCHNULL)
+		{
+			/* IS NULL scan key, but range has no NULLs */
+			if (!bval->bv_allnulls && !bval->bv_hasnulls)
+				return false;
+		}
+		else if (key->sk_flags & SK_SEARCHNOTNULL)
+		{
+			/*
+			 * For IS NOT NULL, we can only skip ranges that are known to
+			 * have only nulls.
+			 */
+			if (bval->bv_allnulls)
+				return false;
+		}
+		else
+		{
+			/*
+			 * Neither IS NULL nor IS NOT NULL was used; assume all indexable
+			 * operators are strict and thus return false with NULL value in
+			 * the scan key.
+			 */
+			return false;
+		}
+	}
+
+	return true;
+}
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 215bc794d3..f4730be3b9 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -110,6 +110,7 @@ brin_inclusion_opcinfo(PG_FUNCTION_ARGS)
 	 */
 	result = palloc0(MAXALIGN(SizeofBrinOpcInfo(3)) + sizeof(InclusionOpaque));
 	result->oi_nstored = 3;
+	result->oi_regular_nulls = true;
 	result->oi_opaque = (InclusionOpaque *)
 		MAXALIGN((char *) result + SizeofBrinOpcInfo(3));
 
@@ -141,7 +142,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
 	BrinDesc   *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
 	BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
 	Datum		newval = PG_GETARG_DATUM(2);
-	bool		isnull = PG_GETARG_BOOL(3);
+	bool		isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_BOOL(3);
 	Oid			colloid = PG_GET_COLLATION();
 	FmgrInfo   *finfo;
 	Datum		result;
@@ -149,18 +150,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
 	AttrNumber	attno;
 	Form_pg_attribute attr;
 
-	/*
-	 * If the new value is null, we record that we saw it if it's the first
-	 * one; otherwise, there's nothing to do.
-	 */
-	if (isnull)
-	{
-		if (column->bv_hasnulls)
-			PG_RETURN_BOOL(false);
-
-		column->bv_hasnulls = true;
-		PG_RETURN_BOOL(true);
-	}
+	Assert(!isnull);
 
 	attno = column->bv_attno;
 	attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -264,63 +254,6 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
 	int			nkeys = PG_GETARG_INT32(3);
 	Oid			colloid = PG_GET_COLLATION();
 	int			keyno;
-	bool		regular_keys = false;
-
-	/*
-	 * First check if there are any IS NULL scan keys, and if we're
-	 * violating them. In that case we can terminate early, without
-	 * inspecting the ranges.
-	 */
-	for (keyno = 0; keyno < nkeys; keyno++)
-	{
-		ScanKey	key = keys[keyno];
-
-		Assert(key->sk_attno == column->bv_attno);
-
-		/* handle IS NULL/IS NOT NULL tests */
-		if (key->sk_flags & SK_ISNULL)
-		{
-			if (key->sk_flags & SK_SEARCHNULL)
-			{
-				if (column->bv_allnulls || column->bv_hasnulls)
-					continue;	/* this key is fine, continue */
-
-				PG_RETURN_BOOL(false);
-			}
-
-			/*
-			 * For IS NOT NULL, we can only skip ranges that are known to have
-			 * only nulls.
-			 */
-			if (key->sk_flags & SK_SEARCHNOTNULL)
-			{
-				if (column->bv_allnulls)
-					PG_RETURN_BOOL(false);
-
-				continue;
-			}
-
-			/*
-			 * Neither IS NULL nor IS NOT NULL was used; assume all indexable
-			 * operators are strict and return false.
-			 */
-			PG_RETURN_BOOL(false);
-		}
-		else
-			/* note we have regular (non-NULL) scan keys */
-			regular_keys = true;
-	}
-
-	/*
-	 * If the page range is all nulls, it cannot possibly be consistent if
-	 * there are some regular scan keys.
-	 */
-	if (column->bv_allnulls && regular_keys)
-		PG_RETURN_BOOL(false);
-
-	/* If there are no regular keys, the page range is considered consistent. */
-	if (!regular_keys)
-		PG_RETURN_BOOL(true);
 
 	/* It has to be checked, if it contains elements that are not mergeable. */
 	if (DatumGetBool(column->bv_values[INCLUSION_UNMERGEABLE]))
@@ -331,9 +264,8 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
 	{
 		ScanKey		key = keys[keyno];
 
-		/* ignore IS NULL/IS NOT NULL tests handled above */
-		if (key->sk_flags & SK_ISNULL)
-			continue;
+		/* NULL keys are handled and filtered-out in bringetbitmap */
+		Assert(!(key->sk_flags & SK_ISNULL));
 
 		/*
 		 * When there are multiple scan keys, failure to meet the
@@ -572,37 +504,11 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
 	Datum		result;
 
 	Assert(col_a->bv_attno == col_b->bv_attno);
-
-	/* Adjust "hasnulls". */
-	if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
-		col_a->bv_hasnulls = true;
-
-	/* If there are no values in B, there's nothing left to do. */
-	if (col_b->bv_allnulls)
-		PG_RETURN_VOID();
+	Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
 
 	attno = col_a->bv_attno;
 	attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
 
-	/*
-	 * Adjust "allnulls".  If A doesn't have values, just copy the values from
-	 * B into A, and we're done.  We cannot run the operators in this case,
-	 * because values in A might contain garbage.  Note we already established
-	 * that B contains values.
-	 */
-	if (col_a->bv_allnulls)
-	{
-		col_a->bv_allnulls = false;
-		col_a->bv_values[INCLUSION_UNION] =
-			datumCopy(col_b->bv_values[INCLUSION_UNION],
-					  attr->attbyval, attr->attlen);
-		col_a->bv_values[INCLUSION_UNMERGEABLE] =
-			col_b->bv_values[INCLUSION_UNMERGEABLE];
-		col_a->bv_values[INCLUSION_CONTAINS_EMPTY] =
-			col_b->bv_values[INCLUSION_CONTAINS_EMPTY];
-		PG_RETURN_VOID();
-	}
-
 	/* If B includes empty elements, mark A similarly, if needed. */
 	if (!DatumGetBool(col_a->bv_values[INCLUSION_CONTAINS_EMPTY]) &&
 		DatumGetBool(col_b->bv_values[INCLUSION_CONTAINS_EMPTY]))
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 12878ff3a0..6c8852d404 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -48,6 +48,7 @@ brin_minmax_opcinfo(PG_FUNCTION_ARGS)
 	result = palloc0(MAXALIGN(SizeofBrinOpcInfo(2)) +
 					 sizeof(MinmaxOpaque));
 	result->oi_nstored = 2;
+	result->oi_regular_nulls = true;
 	result->oi_opaque = (MinmaxOpaque *)
 		MAXALIGN((char *) result + SizeofBrinOpcInfo(2));
 	result->oi_typcache[0] = result->oi_typcache[1] =
@@ -69,7 +70,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
 	BrinDesc   *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
 	BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
 	Datum		newval = PG_GETARG_DATUM(2);
-	bool		isnull = PG_GETARG_DATUM(3);
+	bool		isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_DATUM(3);
 	Oid			colloid = PG_GET_COLLATION();
 	FmgrInfo   *cmpFn;
 	Datum		compar;
@@ -77,18 +78,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
 	Form_pg_attribute attr;
 	AttrNumber	attno;
 
-	/*
-	 * If the new value is null, we record that we saw it if it's the first
-	 * one; otherwise, there's nothing to do.
-	 */
-	if (isnull)
-	{
-		if (column->bv_hasnulls)
-			PG_RETURN_BOOL(false);
-
-		column->bv_hasnulls = true;
-		PG_RETURN_BOOL(true);
-	}
+	Assert(!isnull);
 
 	attno = column->bv_attno;
 	attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -152,72 +142,14 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
 	int			nkeys = PG_GETARG_INT32(3);
 	Oid			colloid = PG_GET_COLLATION();
 	int			keyno;
-	bool		regular_keys = false;
-
-	/*
-	 * First check if there are any IS NULL scan keys, and if we're
-	 * violating them. In that case we can terminate early, without
-	 * inspecting the ranges.
-	 */
-	for (keyno = 0; keyno < nkeys; keyno++)
-	{
-		ScanKey	key = keys[keyno];
-
-		Assert(key->sk_attno == column->bv_attno);
-
-		/* handle IS NULL/IS NOT NULL tests */
-		if (key->sk_flags & SK_ISNULL)
-		{
-			if (key->sk_flags & SK_SEARCHNULL)
-			{
-				if (column->bv_allnulls || column->bv_hasnulls)
-					continue;	/* this key is fine, continue */
-
-				PG_RETURN_BOOL(false);
-			}
-
-			/*
-			 * For IS NOT NULL, we can only skip ranges that are known to have
-			 * only nulls.
-			 */
-			if (key->sk_flags & SK_SEARCHNOTNULL)
-			{
-				if (column->bv_allnulls)
-					PG_RETURN_BOOL(false);
-
-				continue;
-			}
-
-			/*
-			 * Neither IS NULL nor IS NOT NULL was used; assume all indexable
-			 * operators are strict and return false.
-			 */
-			PG_RETURN_BOOL(false);
-		}
-		else
-			/* note we have regular (non-NULL) scan keys */
-			regular_keys = true;
-	}
-
-	/*
-	 * If the page range is all nulls, it cannot possibly be consistent if
-	 * there are some regular scan keys.
-	 */
-	if (column->bv_allnulls && regular_keys)
-		PG_RETURN_BOOL(false);
-
-	/* If there are no regular keys, the page range is considered consistent. */
-	if (!regular_keys)
-		PG_RETURN_BOOL(true);
 
 	/* Check that the range is consistent with all scan keys. */
 	for (keyno = 0; keyno < nkeys; keyno++)
 	{
 		ScanKey	key = keys[keyno];
 
-		/* ignore IS NULL/IS NOT NULL tests handled above */
-		if (key->sk_flags & SK_ISNULL)
-			continue;
+		/* NULL keys are handled and filtered-out in bringetbitmap */
+		Assert(!(key->sk_flags & SK_ISNULL));
 
 		 /*
 		 * When there are multiple scan keys, failure to meet the
@@ -308,34 +240,11 @@ brin_minmax_union(PG_FUNCTION_ARGS)
 	bool		needsadj;
 
 	Assert(col_a->bv_attno == col_b->bv_attno);
-
-	/* Adjust "hasnulls" */
-	if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
-		col_a->bv_hasnulls = true;
-
-	/* If there are no values in B, there's nothing left to do */
-	if (col_b->bv_allnulls)
-		PG_RETURN_VOID();
+	Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
 
 	attno = col_a->bv_attno;
 	attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
 
-	/*
-	 * Adjust "allnulls".  If A doesn't have values, just copy the values from
-	 * B into A, and we're done.  We cannot run the operators in this case,
-	 * because values in A might contain garbage.  Note we already established
-	 * that B contains values.
-	 */
-	if (col_a->bv_allnulls)
-	{
-		col_a->bv_allnulls = false;
-		col_a->bv_values[0] = datumCopy(col_b->bv_values[0],
-										attr->attbyval, attr->attlen);
-		col_a->bv_values[1] = datumCopy(col_b->bv_values[1],
-										attr->attbyval, attr->attlen);
-		PG_RETURN_VOID();
-	}
-
 	/* Adjust minimum, if B's min is less than A's min */
 	finfo = minmax_get_strategy_procinfo(bdesc, attno, attr->atttypid,
 										 BTLessStrategyNumber);
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 78c89a6961..79440ebe7b 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -27,6 +27,9 @@ typedef struct BrinOpcInfo
 	/* Number of columns stored in an index column of this opclass */
 	uint16		oi_nstored;
 
+	/* Regular processing of NULLs in BrinValues? */
+	bool		oi_regular_nulls;
+
 	/* Opaque pointer for the opclass' private use */
 	void	   *oi_opaque;
 
-- 
2.26.2


--------------FA974B75A0C3E9CA18CE7207
Content-Type: text/x-patch; charset=UTF-8;
 name="0003-Optimize-allocations-in-bringetbitmap-20210122.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0003-Optimize-allocations-in-bringetbitmap-20210122.patch"



^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Catalog domain not-null constraints
@ 2023-11-23 06:56 Peter Eisentraut <[email protected]>
  2023-11-23 13:13 ` Re: Catalog domain not-null constraints Aleksander Alekseev <[email protected]>
  2023-11-23 16:38 ` Re: Catalog domain not-null constraints Alvaro Herrera <[email protected]>
  0 siblings, 2 replies; 5+ messages in thread

From: Peter Eisentraut @ 2023-11-23 06:56 UTC (permalink / raw)
  To: pgsql-hackers

This patch set applies the explicit catalog representation of not-null 
constraints introduced by b0e96f3119 for table constraints also to 
domain not-null constraints.

Since there is no inheritance or primary keys etc., this is much simpler 
and just applies the existing infrastructure to domains as well.  As a 
result, domain not-null constraints now appear in the information schema 
correctly.  Another effect is that you can now use the ALTER DOMAIN ... 
ADD/DROP CONSTRAINT syntax for not-null constraints as well.  This makes 
everything consistent overall.

For the most part, I structured the code so that there are now separate 
sibling subroutines for domain check constraints and domain not-null 
constraints.  This seemed to work out best, but one could also consider 
other ways to refactor this.
From c76dd1e21fb2c0a35d45106649788afe2b2b59bd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 23 Nov 2023 07:35:32 +0100
Subject: [PATCH v1 1/2] Add tests for domain-related information schema views

---
 src/test/regress/expected/domain.out | 47 ++++++++++++++++++++++++++++
 src/test/regress/sql/domain.sql      | 24 ++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 6d94e84414..e70aebd70c 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -1207,3 +1207,50 @@ create domain testdomain1 as int constraint unsigned check (value > 0);
 alter domain testdomain1 rename constraint unsigned to unsigned_foo;
 alter domain testdomain1 drop constraint unsigned_foo;
 drop domain testdomain1;
+--
+-- Information schema
+--
+SELECT * FROM information_schema.column_domain_usage
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY domain_name;
+ domain_catalog | domain_schema | domain_name | table_catalog | table_schema | table_name | column_name 
+----------------+---------------+-------------+---------------+--------------+------------+-------------
+ regression     | public        | con         | regression    | public       | domcontest | col1
+ regression     | public        | dom         | regression    | public       | domview    | col1
+ regression     | public        | things      | regression    | public       | thethings  | stuff
+(3 rows)
+
+SELECT * FROM information_schema.domain_constraints
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY constraint_name;
+ constraint_catalog | constraint_schema | constraint_name | domain_catalog | domain_schema | domain_name | is_deferrable | initially_deferred 
+--------------------+-------------------+-----------------+----------------+---------------+-------------+---------------+--------------------
+ regression         | public            | con_check       | regression     | public        | con         | NO            | NO
+ regression         | public            | meow            | regression     | public        | things      | NO            | NO
+ regression         | public            | pos_int_check   | regression     | public        | pos_int     | NO            | NO
+(3 rows)
+
+SELECT * FROM information_schema.domains
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY domain_name;
+ domain_catalog | domain_schema | domain_name | data_type | character_maximum_length | character_octet_length | character_set_catalog | character_set_schema | character_set_name | collation_catalog | collation_schema | collation_name | numeric_precision | numeric_precision_radix | numeric_scale | datetime_precision | interval_type | interval_precision | domain_default | udt_catalog | udt_schema | udt_name | scope_catalog | scope_schema | scope_name | maximum_cardinality | dtd_identifier 
+----------------+---------------+-------------+-----------+--------------------------+------------------------+-----------------------+----------------------+--------------------+-------------------+------------------+----------------+-------------------+-------------------------+---------------+--------------------+---------------+--------------------+----------------+-------------+------------+----------+---------------+--------------+------------+---------------------+----------------
+ regression     | public        | con         | integer   |                          |                        |                       |                      |                    |                   |                  |                |                32 |                       2 |             0 |                    |               |                    |                | regression  | pg_catalog | int4     |               |              |            |                     | 1
+ regression     | public        | dom         | integer   |                          |                        |                       |                      |                    |                   |                  |                |                32 |                       2 |             0 |                    |               |                    |                | regression  | pg_catalog | int4     |               |              |            |                     | 1
+ regression     | public        | pos_int     | integer   |                          |                        |                       |                      |                    |                   |                  |                |                32 |                       2 |             0 |                    |               |                    |                | regression  | pg_catalog | int4     |               |              |            |                     | 1
+ regression     | public        | things      | integer   |                          |                        |                       |                      |                    |                   |                  |                |                32 |                       2 |             0 |                    |               |                    |                | regression  | pg_catalog | int4     |               |              |            |                     | 1
+(4 rows)
+
+SELECT * FROM information_schema.check_constraints
+  WHERE (constraint_schema, constraint_name)
+        IN (SELECT constraint_schema, constraint_name
+            FROM information_schema.domain_constraints
+            WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
+  ORDER BY constraint_name;
+ constraint_catalog | constraint_schema | constraint_name | check_clause 
+--------------------+-------------------+-----------------+--------------
+ regression         | public            | con_check       | (VALUE > 0)
+ regression         | public            | meow            | (VALUE < 11)
+ regression         | public            | pos_int_check   | (VALUE > 0)
+(3 rows)
+
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 745f5d5fd2..813048c19f 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -809,3 +809,27 @@ CREATE TABLE thethings (stuff things);
 alter domain testdomain1 rename constraint unsigned to unsigned_foo;
 alter domain testdomain1 drop constraint unsigned_foo;
 drop domain testdomain1;
+
+
+--
+-- Information schema
+--
+
+SELECT * FROM information_schema.column_domain_usage
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY domain_name;
+
+SELECT * FROM information_schema.domain_constraints
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY constraint_name;
+
+SELECT * FROM information_schema.domains
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY domain_name;
+
+SELECT * FROM information_schema.check_constraints
+  WHERE (constraint_schema, constraint_name)
+        IN (SELECT constraint_schema, constraint_name
+            FROM information_schema.domain_constraints
+            WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
+  ORDER BY constraint_name;

base-commit: 414e75540f058b23377219586abb3008507f7099
-- 
2.42.1


From 778fd05ba7fde5062e64addc98eda5505ef475ca Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 23 Nov 2023 07:35:32 +0100
Subject: [PATCH v1 2/2] Catalog domain not-null constraints

This applies the explicit catalog representation of not-null
constraints introduced by b0e96f3119 for table constraints also to
domain not-null constraints.

TODO: catversion
---
 src/backend/catalog/information_schema.sql |   2 +-
 src/backend/catalog/pg_constraint.c        |  40 +++
 src/backend/commands/typecmds.c            | 313 +++++++++++++++------
 src/backend/utils/cache/typcache.c         |   2 +-
 src/bin/pg_dump/pg_dump.c                  |   2 +-
 src/include/catalog/pg_constraint.h        |   1 +
 src/test/regress/expected/domain.out       |  49 +++-
 src/test/regress/sql/domain.sql            |  26 ++
 8 files changed, 331 insertions(+), 104 deletions(-)

diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index 10b34c3c5b..95a1b34560 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -448,7 +448,7 @@ CREATE VIEW check_constraints AS
     SELECT current_database()::information_schema.sql_identifier AS constraint_catalog,
            rs.nspname::information_schema.sql_identifier AS constraint_schema,
            con.conname::information_schema.sql_identifier AS constraint_name,
-           pg_catalog.format('%s IS NOT NULL', at.attname)::information_schema.character_data AS check_clause
+           pg_catalog.format('%s IS NOT NULL', coalesce(at.attname, 'VALUE'))::information_schema.character_data AS check_clause
      FROM pg_constraint con
             LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
             LEFT JOIN pg_class c ON c.oid = con.conrelid
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index e9d4d6006e..4f1a5e1c84 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -629,6 +629,46 @@ findNotNullConstraint(Oid relid, const char *colname)
 	return findNotNullConstraintAttnum(relid, attnum);
 }
 
+HeapTuple
+findDomainNotNullConstraint(Oid typid)
+{
+	Relation	pg_constraint;
+	HeapTuple	conTup,
+				retval = NULL;
+	SysScanDesc scan;
+	ScanKeyData key;
+
+	pg_constraint = table_open(ConstraintRelationId, AccessShareLock);
+	ScanKeyInit(&key,
+				Anum_pg_constraint_contypid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(typid));
+	scan = systable_beginscan(pg_constraint, ConstraintRelidTypidNameIndexId,
+							  true, NULL, 1, &key);
+
+	while (HeapTupleIsValid(conTup = systable_getnext(scan)))
+	{
+		Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(conTup);
+
+		/*
+		 * We're looking for a NOTNULL constraint that's marked validated.
+		 */
+		if (con->contype != CONSTRAINT_NOTNULL)
+			continue;
+		if (!con->convalidated)
+			continue;
+
+		/* Found it */
+		retval = heap_copytuple(conTup);
+		break;
+	}
+
+	systable_endscan(scan);
+	table_close(pg_constraint, AccessShareLock);
+
+	return retval;
+}
+
 /*
  * Given a pg_constraint tuple for a not-null constraint, return the column
  * number it is for.
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index aaf9728697..57389be9ed 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -126,15 +126,19 @@ static Oid	findTypeSubscriptingFunction(List *procname, Oid typeOid);
 static Oid	findRangeSubOpclass(List *opcname, Oid subtype);
 static Oid	findRangeCanonicalFunction(List *procname, Oid typeOid);
 static Oid	findRangeSubtypeDiffFunction(List *procname, Oid subtype);
-static void validateDomainConstraint(Oid domainoid, char *ccbin);
+static void validateDomainCheckConstraint(Oid domainoid, char *ccbin);
+static void validateDomainNotNullConstraint(Oid domainoid);
 static List *get_rels_with_domain(Oid domainOid, LOCKMODE lockmode);
 static void checkEnumOwner(HeapTuple tup);
-static char *domainAddConstraint(Oid domainOid, Oid domainNamespace,
-								 Oid baseTypeOid,
-								 int typMod, Constraint *constr,
-								 const char *domainName, ObjectAddress *constrAddr);
+static char *domainAddCheckConstraint(Oid domainOid, Oid domainNamespace,
+									  Oid baseTypeOid,
+									  int typMod, Constraint *constr,
+									  const char *domainName, ObjectAddress *constrAddr);
 static Node *replace_domain_constraint_value(ParseState *pstate,
 											 ColumnRef *cref);
+static void domainAddNotNullConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
+									   int typMod, Constraint *constr,
+									   const char *domainName, ObjectAddress *constrAddr);
 static void AlterTypeRecurse(Oid typeOid, bool isImplicitArray,
 							 HeapTuple tup, Relation catalog,
 							 AlterTypeRecurseParams *atparams);
@@ -1105,9 +1109,15 @@ DefineDomain(CreateDomainStmt *stmt)
 		switch (constr->contype)
 		{
 			case CONSTR_CHECK:
-				domainAddConstraint(address.objectId, domainNamespace,
-									basetypeoid, basetypeMod,
-									constr, domainName, NULL);
+				domainAddCheckConstraint(address.objectId, domainNamespace,
+										 basetypeoid, basetypeMod,
+										 constr, domainName, NULL);
+				break;
+
+			case CONSTR_NOTNULL:
+				domainAddNotNullConstraint(address.objectId, domainNamespace,
+										   basetypeoid, basetypeMod,
+										   constr, domainName, NULL);
 				break;
 
 				/* Other constraint types were fully processed above */
@@ -2723,66 +2733,32 @@ AlterDomainNotNull(List *names, bool notNull)
 		return address;
 	}
 
-	/* Adding a NOT NULL constraint requires checking existing columns */
 	if (notNull)
 	{
-		List	   *rels;
-		ListCell   *rt;
+		Constraint *constr;
 
-		/* Fetch relation list with attributes based on this domain */
-		/* ShareLock is sufficient to prevent concurrent data changes */
+		constr = makeNode(Constraint);
+		constr->contype = CONSTR_NOTNULL;
+		constr->initially_valid = true;
+		constr->location = -1;
 
-		rels = get_rels_with_domain(domainoid, ShareLock);
-
-		foreach(rt, rels)
-		{
-			RelToCheck *rtc = (RelToCheck *) lfirst(rt);
-			Relation	testrel = rtc->rel;
-			TupleDesc	tupdesc = RelationGetDescr(testrel);
-			TupleTableSlot *slot;
-			TableScanDesc scan;
-			Snapshot	snapshot;
-
-			/* Scan all tuples in this relation */
-			snapshot = RegisterSnapshot(GetLatestSnapshot());
-			scan = table_beginscan(testrel, snapshot, 0, NULL);
-			slot = table_slot_create(testrel, NULL);
-			while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
-			{
-				int			i;
+		domainAddNotNullConstraint(domainoid, typTup->typnamespace,
+								   typTup->typbasetype, typTup->typtypmod,
+								   constr, NameStr(typTup->typname), NULL);
 
-				/* Test attributes that are of the domain */
-				for (i = 0; i < rtc->natts; i++)
-				{
-					int			attnum = rtc->atts[i];
-					Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
+		validateDomainNotNullConstraint(domainoid);
+	}
+	else
+	{
+		HeapTuple	conTup;
+		ObjectAddress conobj;
 
-					if (slot_attisnull(slot, attnum))
-					{
-						/*
-						 * In principle the auxiliary information for this
-						 * error should be errdatatype(), but errtablecol()
-						 * seems considerably more useful in practice.  Since
-						 * this code only executes in an ALTER DOMAIN command,
-						 * the client should already know which domain is in
-						 * question.
-						 */
-						ereport(ERROR,
-								(errcode(ERRCODE_NOT_NULL_VIOLATION),
-								 errmsg("column \"%s\" of table \"%s\" contains null values",
-										NameStr(attr->attname),
-										RelationGetRelationName(testrel)),
-								 errtablecol(testrel, attnum)));
-					}
-				}
-			}
-			ExecDropSingleTupleTableSlot(slot);
-			table_endscan(scan);
-			UnregisterSnapshot(snapshot);
+		conTup = findDomainNotNullConstraint(domainoid);
+		if (conTup == NULL)
+			elog(ERROR, "could not find not-null constraint on domain \"%s\"", NameStr(typTup->typname));
 
-			/* Close each rel after processing, but keep lock */
-			table_close(testrel, NoLock);
-		}
+		ObjectAddressSet(conobj, ConstraintRelationId, ((Form_pg_constraint) GETSTRUCT(conTup))->oid);
+		performDeletion(&conobj, DROP_RESTRICT, 0);
 	}
 
 	/*
@@ -2863,10 +2839,17 @@ AlterDomainDropConstraint(List *names, const char *constrName,
 	/* There can be at most one matching row */
 	if ((contup = systable_getnext(conscan)) != NULL)
 	{
+		Form_pg_constraint construct = (Form_pg_constraint) GETSTRUCT(contup);
 		ObjectAddress conobj;
 
+		if (construct->contype == CONSTRAINT_NOTNULL)
+		{
+			((Form_pg_type) GETSTRUCT(tup))->typnotnull = false;
+			CatalogTupleUpdate(rel, &tup->t_self, tup);
+		}
+
 		conobj.classId = ConstraintRelationId;
-		conobj.objectId = ((Form_pg_constraint) GETSTRUCT(contup))->oid;
+		conobj.objectId = construct->oid;
 		conobj.objectSubId = 0;
 
 		performDeletion(&conobj, behavior, 0);
@@ -2947,6 +2930,7 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
 	switch (constr->contype)
 	{
 		case CONSTR_CHECK:
+		case CONSTR_NOTNULL:
 			/* processed below */
 			break;
 
@@ -2989,29 +2973,44 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
 			break;
 	}
 
-	/*
-	 * Since all other constraint types throw errors, this must be a check
-	 * constraint.  First, process the constraint expression and add an entry
-	 * to pg_constraint.
-	 */
+	if (constr->contype == CONSTR_CHECK)
+	{
+		/*
+		 * First, process the constraint expression and add an entry
+		 * to pg_constraint.
+		 */
 
-	ccbin = domainAddConstraint(domainoid, typTup->typnamespace,
-								typTup->typbasetype, typTup->typtypmod,
-								constr, NameStr(typTup->typname), constrAddr);
+		ccbin = domainAddCheckConstraint(domainoid, typTup->typnamespace,
+										 typTup->typbasetype, typTup->typtypmod,
+										 constr, NameStr(typTup->typname), constrAddr);
 
-	/*
-	 * If requested to validate the constraint, test all values stored in the
-	 * attributes based on the domain the constraint is being added to.
-	 */
-	if (!constr->skip_validation)
-		validateDomainConstraint(domainoid, ccbin);
 
-	/*
-	 * We must send out an sinval message for the domain, to ensure that any
-	 * dependent plans get rebuilt.  Since this command doesn't change the
-	 * domain's pg_type row, that won't happen automatically; do it manually.
-	 */
-	CacheInvalidateHeapTuple(typrel, tup, NULL);
+		/*
+		 * If requested to validate the constraint, test all values stored in the
+		 * attributes based on the domain the constraint is being added to.
+		 */
+		if (!constr->skip_validation)
+			validateDomainCheckConstraint(domainoid, ccbin);
+
+		/*
+		 * We must send out an sinval message for the domain, to ensure that any
+		 * dependent plans get rebuilt.  Since this command doesn't change the
+		 * domain's pg_type row, that won't happen automatically; do it manually.
+		 */
+		CacheInvalidateHeapTuple(typrel, tup, NULL);
+	}
+	else if (constr->contype == CONSTR_NOTNULL)
+	{
+		domainAddNotNullConstraint(domainoid, typTup->typnamespace,
+								   typTup->typbasetype, typTup->typtypmod,
+								   constr, NameStr(typTup->typname), constrAddr);
+
+		if (!constr->skip_validation)
+			validateDomainNotNullConstraint(domainoid);
+
+		typTup->typnotnull = true;
+		CatalogTupleUpdate(typrel, &tup->t_self, tup);
+	}
 
 	ObjectAddressSet(address, TypeRelationId, domainoid);
 
@@ -3096,7 +3095,7 @@ AlterDomainValidateConstraint(List *names, const char *constrName)
 	val = SysCacheGetAttrNotNull(CONSTROID, tuple, Anum_pg_constraint_conbin);
 	conbin = TextDatumGetCString(val);
 
-	validateDomainConstraint(domainoid, conbin);
+	validateDomainCheckConstraint(domainoid, conbin);
 
 	/*
 	 * Now update the catalog, while we have the door open.
@@ -3123,7 +3122,69 @@ AlterDomainValidateConstraint(List *names, const char *constrName)
 }
 
 static void
-validateDomainConstraint(Oid domainoid, char *ccbin)
+validateDomainNotNullConstraint(Oid domainoid)
+{
+	List	   *rels;
+	ListCell   *rt;
+
+	/* Fetch relation list with attributes based on this domain */
+	/* ShareLock is sufficient to prevent concurrent data changes */
+
+	rels = get_rels_with_domain(domainoid, ShareLock);
+
+	foreach(rt, rels)
+	{
+		RelToCheck *rtc = (RelToCheck *) lfirst(rt);
+		Relation	testrel = rtc->rel;
+		TupleDesc	tupdesc = RelationGetDescr(testrel);
+		TupleTableSlot *slot;
+		TableScanDesc scan;
+		Snapshot	snapshot;
+
+		/* Scan all tuples in this relation */
+		snapshot = RegisterSnapshot(GetLatestSnapshot());
+		scan = table_beginscan(testrel, snapshot, 0, NULL);
+		slot = table_slot_create(testrel, NULL);
+		while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
+		{
+			int			i;
+
+			/* Test attributes that are of the domain */
+			for (i = 0; i < rtc->natts; i++)
+			{
+				int			attnum = rtc->atts[i];
+				Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
+
+				if (slot_attisnull(slot, attnum))
+				{
+					/*
+					 * In principle the auxiliary information for this
+					 * error should be errdatatype(), but errtablecol()
+					 * seems considerably more useful in practice.  Since
+					 * this code only executes in an ALTER DOMAIN command,
+					 * the client should already know which domain is in
+					 * question.
+					 */
+					ereport(ERROR,
+							(errcode(ERRCODE_NOT_NULL_VIOLATION),
+							 errmsg("column \"%s\" of table \"%s\" contains null values",
+									NameStr(attr->attname),
+									RelationGetRelationName(testrel)),
+							 errtablecol(testrel, attnum)));
+				}
+			}
+		}
+		ExecDropSingleTupleTableSlot(slot);
+		table_endscan(scan);
+		UnregisterSnapshot(snapshot);
+
+		/* Close each rel after processing, but keep lock */
+		table_close(testrel, NoLock);
+	}
+}
+
+static void
+validateDomainCheckConstraint(Oid domainoid, char *ccbin)
 {
 	Expr	   *expr = (Expr *) stringToNode(ccbin);
 	List	   *rels;
@@ -3429,12 +3490,12 @@ checkDomainOwner(HeapTuple tup)
 }
 
 /*
- * domainAddConstraint - code shared between CREATE and ALTER DOMAIN
+ * domainAddCheckConstraint - code shared between CREATE and ALTER DOMAIN
  */
 static char *
-domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
-					int typMod, Constraint *constr,
-					const char *domainName, ObjectAddress *constrAddr)
+domainAddCheckConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
+						 int typMod, Constraint *constr,
+						 const char *domainName, ObjectAddress *constrAddr)
 {
 	Node	   *expr;
 	char	   *ccbin;
@@ -3442,6 +3503,8 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
 	CoerceToDomainValue *domVal;
 	Oid			ccoid;
 
+	Assert(constr->contype == CONSTR_CHECK);
+
 	/*
 	 * Assign or validate constraint name
 	 */
@@ -3561,7 +3624,7 @@ replace_domain_constraint_value(ParseState *pstate, ColumnRef *cref)
 {
 	/*
 	 * Check for a reference to "value", and if that's what it is, replace
-	 * with a CoerceToDomainValue as prepared for us by domainAddConstraint.
+	 * with a CoerceToDomainValue as prepared for us by domainAddCheckConstraint.
 	 * (We handle VALUE as a name, not a keyword, to avoid breaking a lot of
 	 * applications that have used VALUE as a column name in the past.)
 	 */
@@ -3583,6 +3646,78 @@ replace_domain_constraint_value(ParseState *pstate, ColumnRef *cref)
 	return NULL;
 }
 
+/*
+ * domainAddNotNullConstraint - code shared between CREATE and ALTER DOMAIN
+ */
+static void
+domainAddNotNullConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
+						   int typMod, Constraint *constr,
+						   const char *domainName, ObjectAddress *constrAddr)
+{
+	Oid			ccoid;
+
+	Assert(constr->contype == CONSTR_NOTNULL);
+
+	/*
+	 * Assign or validate constraint name
+	 */
+	if (constr->conname)
+	{
+		if (ConstraintNameIsUsed(CONSTRAINT_DOMAIN,
+								 domainOid,
+								 constr->conname))
+			ereport(ERROR,
+					(errcode(ERRCODE_DUPLICATE_OBJECT),
+					 errmsg("constraint \"%s\" for domain \"%s\" already exists",
+							constr->conname, domainName)));
+	}
+	else
+		constr->conname = ChooseConstraintName(domainName,
+											   NULL,
+											   "not_null",
+											   domainNamespace,
+											   NIL);
+
+	/*
+	 * Store the constraint in pg_constraint
+	 */
+	ccoid =
+		CreateConstraintEntry(constr->conname,	/* Constraint Name */
+							  domainNamespace,	/* namespace */
+							  CONSTRAINT_NOTNULL, /* Constraint Type */
+							  false,	/* Is Deferrable */
+							  false,	/* Is Deferred */
+							  !constr->skip_validation, /* Is Validated */
+							  InvalidOid,	/* no parent constraint */
+							  InvalidOid,	/* not a relation constraint */
+							  NULL,
+							  0,
+							  0,
+							  domainOid,	/* domain constraint */
+							  InvalidOid,	/* no associated index */
+							  InvalidOid,	/* Foreign key fields */
+							  NULL,
+							  NULL,
+							  NULL,
+							  NULL,
+							  0,
+							  ' ',
+							  ' ',
+							  NULL,
+							  0,
+							  ' ',
+							  NULL, /* not an exclusion constraint */
+							  NULL,
+							  NULL,
+							  true, /* is local */
+							  0,	/* inhcount */
+							  false,	/* connoinherit */
+							  false);	/* is_internal */
+
+	if (constrAddr)
+		ObjectAddressSet(*constrAddr, ConstraintRelationId, ccoid);
+}
+
 
 /*
  * Execute ALTER TYPE RENAME
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 608cd5e8e4..ee1adff932 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -1068,7 +1068,7 @@ load_domaintype_info(TypeCacheEntry *typentry)
 			Expr	   *check_expr;
 			DomainConstraintState *r;
 
-			/* Ignore non-CHECK constraints (presently, shouldn't be any) */
+			/* Ignore non-CHECK constraints */
 			if (c->contype != CONSTRAINT_CHECK)
 				continue;
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 34fd0a86e9..16e9528d6a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7552,7 +7552,7 @@ getDomainConstraints(Archive *fout, TypeInfo *tyinfo)
 							 "pg_catalog.pg_get_constraintdef(oid) AS consrc, "
 							 "convalidated "
 							 "FROM pg_catalog.pg_constraint "
-							 "WHERE contypid = $1 "
+							 "WHERE contypid = $1 AND contype = 'c' "
 							 "ORDER BY conname");
 
 		ExecuteSqlStatement(fout, query->data);
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index a026b42515..c007fe81a8 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -247,6 +247,7 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
 
 extern HeapTuple findNotNullConstraintAttnum(Oid relid, AttrNumber attnum);
 extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
+extern HeapTuple findDomainNotNullConstraint(Oid typid);
 extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
 extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
 									  bool is_no_inherit);
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index e70aebd70c..799b268ac7 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -798,6 +798,29 @@ alter domain con drop constraint nonexistent;
 ERROR:  constraint "nonexistent" of domain "con" does not exist
 alter domain con drop constraint if exists nonexistent;
 NOTICE:  constraint "nonexistent" of domain "con" does not exist, skipping
+-- not-null constraints
+create domain connotnull integer;
+create table domconnotnulltest
+( col1 connotnull
+, col2 connotnull
+);
+insert into domconnotnulltest default values;
+alter domain connotnull add not null value; -- fails
+ERROR:  column "col1" of table "domconnotnulltest" contains null values
+update domconnotnulltest set col1 = 5;
+alter domain connotnull add not null value; -- fails
+ERROR:  column "col2" of table "domconnotnulltest" contains null values
+update domconnotnulltest set col2 = 6;
+alter domain connotnull add constraint constr1 not null value;
+update domconnotnulltest set col1 = null; -- fails
+ERROR:  domain connotnull does not allow null values
+alter domain connotnull drop constraint constr1;
+update domconnotnulltest set col1 = null;
+drop domain connotnull cascade;
+NOTICE:  drop cascades to 2 other objects
+DETAIL:  drop cascades to column col2 of table domconnotnulltest
+drop cascades to column col1 of table domconnotnulltest
+drop table domconnotnulltest;
 -- Test ALTER DOMAIN .. CONSTRAINT .. NOT VALID
 create domain things AS INT;
 CREATE TABLE thethings (stuff things);
@@ -1223,12 +1246,13 @@ SELECT * FROM information_schema.column_domain_usage
 SELECT * FROM information_schema.domain_constraints
   WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
   ORDER BY constraint_name;
- constraint_catalog | constraint_schema | constraint_name | domain_catalog | domain_schema | domain_name | is_deferrable | initially_deferred 
---------------------+-------------------+-----------------+----------------+---------------+-------------+---------------+--------------------
- regression         | public            | con_check       | regression     | public        | con         | NO            | NO
- regression         | public            | meow            | regression     | public        | things      | NO            | NO
- regression         | public            | pos_int_check   | regression     | public        | pos_int     | NO            | NO
-(3 rows)
+ constraint_catalog | constraint_schema | constraint_name  | domain_catalog | domain_schema | domain_name | is_deferrable | initially_deferred 
+--------------------+-------------------+------------------+----------------+---------------+-------------+---------------+--------------------
+ regression         | public            | con_check        | regression     | public        | con         | NO            | NO
+ regression         | public            | meow             | regression     | public        | things      | NO            | NO
+ regression         | public            | pos_int_check    | regression     | public        | pos_int     | NO            | NO
+ regression         | public            | pos_int_not_null | regression     | public        | pos_int     | NO            | NO
+(4 rows)
 
 SELECT * FROM information_schema.domains
   WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
@@ -1247,10 +1271,11 @@ SELECT * FROM information_schema.check_constraints
             FROM information_schema.domain_constraints
             WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
   ORDER BY constraint_name;
- constraint_catalog | constraint_schema | constraint_name | check_clause 
---------------------+-------------------+-----------------+--------------
- regression         | public            | con_check       | (VALUE > 0)
- regression         | public            | meow            | (VALUE < 11)
- regression         | public            | pos_int_check   | (VALUE > 0)
-(3 rows)
+ constraint_catalog | constraint_schema | constraint_name  |   check_clause    
+--------------------+-------------------+------------------+-------------------
+ regression         | public            | con_check        | (VALUE > 0)
+ regression         | public            | meow             | (VALUE < 11)
+ regression         | public            | pos_int_check    | (VALUE > 0)
+ regression         | public            | pos_int_not_null | VALUE IS NOT NULL
+(4 rows)
 
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 813048c19f..0e71f04004 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -469,6 +469,32 @@
 alter domain con drop constraint nonexistent;
 alter domain con drop constraint if exists nonexistent;
 
+-- not-null constraints
+create domain connotnull integer;
+create table domconnotnulltest
+( col1 connotnull
+, col2 connotnull
+);
+
+insert into domconnotnulltest default values;
+alter domain connotnull add not null value; -- fails
+
+update domconnotnulltest set col1 = 5;
+alter domain connotnull add not null value; -- fails
+
+update domconnotnulltest set col2 = 6;
+
+alter domain connotnull add constraint constr1 not null value;
+
+update domconnotnulltest set col1 = null; -- fails
+
+alter domain connotnull drop constraint constr1;
+
+update domconnotnulltest set col1 = null;
+
+drop domain connotnull cascade;
+drop table domconnotnulltest;
+
 -- Test ALTER DOMAIN .. CONSTRAINT .. NOT VALID
 create domain things AS INT;
 CREATE TABLE thethings (stuff things);
-- 
2.42.1



Attachments:

  [text/plain] v1-0001-Add-tests-for-domain-related-information-schema-v.patch (7.2K, ../../[email protected]/2-v1-0001-Add-tests-for-domain-related-information-schema-v.patch)
  download | inline diff:
From c76dd1e21fb2c0a35d45106649788afe2b2b59bd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 23 Nov 2023 07:35:32 +0100
Subject: [PATCH v1 1/2] Add tests for domain-related information schema views

---
 src/test/regress/expected/domain.out | 47 ++++++++++++++++++++++++++++
 src/test/regress/sql/domain.sql      | 24 ++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 6d94e84414..e70aebd70c 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -1207,3 +1207,50 @@ create domain testdomain1 as int constraint unsigned check (value > 0);
 alter domain testdomain1 rename constraint unsigned to unsigned_foo;
 alter domain testdomain1 drop constraint unsigned_foo;
 drop domain testdomain1;
+--
+-- Information schema
+--
+SELECT * FROM information_schema.column_domain_usage
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY domain_name;
+ domain_catalog | domain_schema | domain_name | table_catalog | table_schema | table_name | column_name 
+----------------+---------------+-------------+---------------+--------------+------------+-------------
+ regression     | public        | con         | regression    | public       | domcontest | col1
+ regression     | public        | dom         | regression    | public       | domview    | col1
+ regression     | public        | things      | regression    | public       | thethings  | stuff
+(3 rows)
+
+SELECT * FROM information_schema.domain_constraints
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY constraint_name;
+ constraint_catalog | constraint_schema | constraint_name | domain_catalog | domain_schema | domain_name | is_deferrable | initially_deferred 
+--------------------+-------------------+-----------------+----------------+---------------+-------------+---------------+--------------------
+ regression         | public            | con_check       | regression     | public        | con         | NO            | NO
+ regression         | public            | meow            | regression     | public        | things      | NO            | NO
+ regression         | public            | pos_int_check   | regression     | public        | pos_int     | NO            | NO
+(3 rows)
+
+SELECT * FROM information_schema.domains
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY domain_name;
+ domain_catalog | domain_schema | domain_name | data_type | character_maximum_length | character_octet_length | character_set_catalog | character_set_schema | character_set_name | collation_catalog | collation_schema | collation_name | numeric_precision | numeric_precision_radix | numeric_scale | datetime_precision | interval_type | interval_precision | domain_default | udt_catalog | udt_schema | udt_name | scope_catalog | scope_schema | scope_name | maximum_cardinality | dtd_identifier 
+----------------+---------------+-------------+-----------+--------------------------+------------------------+-----------------------+----------------------+--------------------+-------------------+------------------+----------------+-------------------+-------------------------+---------------+--------------------+---------------+--------------------+----------------+-------------+------------+----------+---------------+--------------+------------+---------------------+----------------
+ regression     | public        | con         | integer   |                          |                        |                       |                      |                    |                   |                  |                |                32 |                       2 |             0 |                    |               |                    |                | regression  | pg_catalog | int4     |               |              |            |                     | 1
+ regression     | public        | dom         | integer   |                          |                        |                       |                      |                    |                   |                  |                |                32 |                       2 |             0 |                    |               |                    |                | regression  | pg_catalog | int4     |               |              |            |                     | 1
+ regression     | public        | pos_int     | integer   |                          |                        |                       |                      |                    |                   |                  |                |                32 |                       2 |             0 |                    |               |                    |                | regression  | pg_catalog | int4     |               |              |            |                     | 1
+ regression     | public        | things      | integer   |                          |                        |                       |                      |                    |                   |                  |                |                32 |                       2 |             0 |                    |               |                    |                | regression  | pg_catalog | int4     |               |              |            |                     | 1
+(4 rows)
+
+SELECT * FROM information_schema.check_constraints
+  WHERE (constraint_schema, constraint_name)
+        IN (SELECT constraint_schema, constraint_name
+            FROM information_schema.domain_constraints
+            WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
+  ORDER BY constraint_name;
+ constraint_catalog | constraint_schema | constraint_name | check_clause 
+--------------------+-------------------+-----------------+--------------
+ regression         | public            | con_check       | (VALUE > 0)
+ regression         | public            | meow            | (VALUE < 11)
+ regression         | public            | pos_int_check   | (VALUE > 0)
+(3 rows)
+
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 745f5d5fd2..813048c19f 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -809,3 +809,27 @@ CREATE TABLE thethings (stuff things);
 alter domain testdomain1 rename constraint unsigned to unsigned_foo;
 alter domain testdomain1 drop constraint unsigned_foo;
 drop domain testdomain1;
+
+
+--
+-- Information schema
+--
+
+SELECT * FROM information_schema.column_domain_usage
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY domain_name;
+
+SELECT * FROM information_schema.domain_constraints
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY constraint_name;
+
+SELECT * FROM information_schema.domains
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY domain_name;
+
+SELECT * FROM information_schema.check_constraints
+  WHERE (constraint_schema, constraint_name)
+        IN (SELECT constraint_schema, constraint_name
+            FROM information_schema.domain_constraints
+            WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
+  ORDER BY constraint_name;

base-commit: 414e75540f058b23377219586abb3008507f7099
-- 
2.42.1



  [text/plain] v1-0002-Catalog-domain-not-null-constraints.patch (24.1K, ../../[email protected]/3-v1-0002-Catalog-domain-not-null-constraints.patch)
  download | inline diff:
From 778fd05ba7fde5062e64addc98eda5505ef475ca Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Thu, 23 Nov 2023 07:35:32 +0100
Subject: [PATCH v1 2/2] Catalog domain not-null constraints

This applies the explicit catalog representation of not-null
constraints introduced by b0e96f3119 for table constraints also to
domain not-null constraints.

TODO: catversion
---
 src/backend/catalog/information_schema.sql |   2 +-
 src/backend/catalog/pg_constraint.c        |  40 +++
 src/backend/commands/typecmds.c            | 313 +++++++++++++++------
 src/backend/utils/cache/typcache.c         |   2 +-
 src/bin/pg_dump/pg_dump.c                  |   2 +-
 src/include/catalog/pg_constraint.h        |   1 +
 src/test/regress/expected/domain.out       |  49 +++-
 src/test/regress/sql/domain.sql            |  26 ++
 8 files changed, 331 insertions(+), 104 deletions(-)

diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index 10b34c3c5b..95a1b34560 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -448,7 +448,7 @@ CREATE VIEW check_constraints AS
     SELECT current_database()::information_schema.sql_identifier AS constraint_catalog,
            rs.nspname::information_schema.sql_identifier AS constraint_schema,
            con.conname::information_schema.sql_identifier AS constraint_name,
-           pg_catalog.format('%s IS NOT NULL', at.attname)::information_schema.character_data AS check_clause
+           pg_catalog.format('%s IS NOT NULL', coalesce(at.attname, 'VALUE'))::information_schema.character_data AS check_clause
      FROM pg_constraint con
             LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
             LEFT JOIN pg_class c ON c.oid = con.conrelid
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index e9d4d6006e..4f1a5e1c84 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -629,6 +629,46 @@ findNotNullConstraint(Oid relid, const char *colname)
 	return findNotNullConstraintAttnum(relid, attnum);
 }
 
+HeapTuple
+findDomainNotNullConstraint(Oid typid)
+{
+	Relation	pg_constraint;
+	HeapTuple	conTup,
+				retval = NULL;
+	SysScanDesc scan;
+	ScanKeyData key;
+
+	pg_constraint = table_open(ConstraintRelationId, AccessShareLock);
+	ScanKeyInit(&key,
+				Anum_pg_constraint_contypid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(typid));
+	scan = systable_beginscan(pg_constraint, ConstraintRelidTypidNameIndexId,
+							  true, NULL, 1, &key);
+
+	while (HeapTupleIsValid(conTup = systable_getnext(scan)))
+	{
+		Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(conTup);
+
+		/*
+		 * We're looking for a NOTNULL constraint that's marked validated.
+		 */
+		if (con->contype != CONSTRAINT_NOTNULL)
+			continue;
+		if (!con->convalidated)
+			continue;
+
+		/* Found it */
+		retval = heap_copytuple(conTup);
+		break;
+	}
+
+	systable_endscan(scan);
+	table_close(pg_constraint, AccessShareLock);
+
+	return retval;
+}
+
 /*
  * Given a pg_constraint tuple for a not-null constraint, return the column
  * number it is for.
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index aaf9728697..57389be9ed 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -126,15 +126,19 @@ static Oid	findTypeSubscriptingFunction(List *procname, Oid typeOid);
 static Oid	findRangeSubOpclass(List *opcname, Oid subtype);
 static Oid	findRangeCanonicalFunction(List *procname, Oid typeOid);
 static Oid	findRangeSubtypeDiffFunction(List *procname, Oid subtype);
-static void validateDomainConstraint(Oid domainoid, char *ccbin);
+static void validateDomainCheckConstraint(Oid domainoid, char *ccbin);
+static void validateDomainNotNullConstraint(Oid domainoid);
 static List *get_rels_with_domain(Oid domainOid, LOCKMODE lockmode);
 static void checkEnumOwner(HeapTuple tup);
-static char *domainAddConstraint(Oid domainOid, Oid domainNamespace,
-								 Oid baseTypeOid,
-								 int typMod, Constraint *constr,
-								 const char *domainName, ObjectAddress *constrAddr);
+static char *domainAddCheckConstraint(Oid domainOid, Oid domainNamespace,
+									  Oid baseTypeOid,
+									  int typMod, Constraint *constr,
+									  const char *domainName, ObjectAddress *constrAddr);
 static Node *replace_domain_constraint_value(ParseState *pstate,
 											 ColumnRef *cref);
+static void domainAddNotNullConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
+									   int typMod, Constraint *constr,
+									   const char *domainName, ObjectAddress *constrAddr);
 static void AlterTypeRecurse(Oid typeOid, bool isImplicitArray,
 							 HeapTuple tup, Relation catalog,
 							 AlterTypeRecurseParams *atparams);
@@ -1105,9 +1109,15 @@ DefineDomain(CreateDomainStmt *stmt)
 		switch (constr->contype)
 		{
 			case CONSTR_CHECK:
-				domainAddConstraint(address.objectId, domainNamespace,
-									basetypeoid, basetypeMod,
-									constr, domainName, NULL);
+				domainAddCheckConstraint(address.objectId, domainNamespace,
+										 basetypeoid, basetypeMod,
+										 constr, domainName, NULL);
+				break;
+
+			case CONSTR_NOTNULL:
+				domainAddNotNullConstraint(address.objectId, domainNamespace,
+										   basetypeoid, basetypeMod,
+										   constr, domainName, NULL);
 				break;
 
 				/* Other constraint types were fully processed above */
@@ -2723,66 +2733,32 @@ AlterDomainNotNull(List *names, bool notNull)
 		return address;
 	}
 
-	/* Adding a NOT NULL constraint requires checking existing columns */
 	if (notNull)
 	{
-		List	   *rels;
-		ListCell   *rt;
+		Constraint *constr;
 
-		/* Fetch relation list with attributes based on this domain */
-		/* ShareLock is sufficient to prevent concurrent data changes */
+		constr = makeNode(Constraint);
+		constr->contype = CONSTR_NOTNULL;
+		constr->initially_valid = true;
+		constr->location = -1;
 
-		rels = get_rels_with_domain(domainoid, ShareLock);
-
-		foreach(rt, rels)
-		{
-			RelToCheck *rtc = (RelToCheck *) lfirst(rt);
-			Relation	testrel = rtc->rel;
-			TupleDesc	tupdesc = RelationGetDescr(testrel);
-			TupleTableSlot *slot;
-			TableScanDesc scan;
-			Snapshot	snapshot;
-
-			/* Scan all tuples in this relation */
-			snapshot = RegisterSnapshot(GetLatestSnapshot());
-			scan = table_beginscan(testrel, snapshot, 0, NULL);
-			slot = table_slot_create(testrel, NULL);
-			while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
-			{
-				int			i;
+		domainAddNotNullConstraint(domainoid, typTup->typnamespace,
+								   typTup->typbasetype, typTup->typtypmod,
+								   constr, NameStr(typTup->typname), NULL);
 
-				/* Test attributes that are of the domain */
-				for (i = 0; i < rtc->natts; i++)
-				{
-					int			attnum = rtc->atts[i];
-					Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
+		validateDomainNotNullConstraint(domainoid);
+	}
+	else
+	{
+		HeapTuple	conTup;
+		ObjectAddress conobj;
 
-					if (slot_attisnull(slot, attnum))
-					{
-						/*
-						 * In principle the auxiliary information for this
-						 * error should be errdatatype(), but errtablecol()
-						 * seems considerably more useful in practice.  Since
-						 * this code only executes in an ALTER DOMAIN command,
-						 * the client should already know which domain is in
-						 * question.
-						 */
-						ereport(ERROR,
-								(errcode(ERRCODE_NOT_NULL_VIOLATION),
-								 errmsg("column \"%s\" of table \"%s\" contains null values",
-										NameStr(attr->attname),
-										RelationGetRelationName(testrel)),
-								 errtablecol(testrel, attnum)));
-					}
-				}
-			}
-			ExecDropSingleTupleTableSlot(slot);
-			table_endscan(scan);
-			UnregisterSnapshot(snapshot);
+		conTup = findDomainNotNullConstraint(domainoid);
+		if (conTup == NULL)
+			elog(ERROR, "could not find not-null constraint on domain \"%s\"", NameStr(typTup->typname));
 
-			/* Close each rel after processing, but keep lock */
-			table_close(testrel, NoLock);
-		}
+		ObjectAddressSet(conobj, ConstraintRelationId, ((Form_pg_constraint) GETSTRUCT(conTup))->oid);
+		performDeletion(&conobj, DROP_RESTRICT, 0);
 	}
 
 	/*
@@ -2863,10 +2839,17 @@ AlterDomainDropConstraint(List *names, const char *constrName,
 	/* There can be at most one matching row */
 	if ((contup = systable_getnext(conscan)) != NULL)
 	{
+		Form_pg_constraint construct = (Form_pg_constraint) GETSTRUCT(contup);
 		ObjectAddress conobj;
 
+		if (construct->contype == CONSTRAINT_NOTNULL)
+		{
+			((Form_pg_type) GETSTRUCT(tup))->typnotnull = false;
+			CatalogTupleUpdate(rel, &tup->t_self, tup);
+		}
+
 		conobj.classId = ConstraintRelationId;
-		conobj.objectId = ((Form_pg_constraint) GETSTRUCT(contup))->oid;
+		conobj.objectId = construct->oid;
 		conobj.objectSubId = 0;
 
 		performDeletion(&conobj, behavior, 0);
@@ -2947,6 +2930,7 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
 	switch (constr->contype)
 	{
 		case CONSTR_CHECK:
+		case CONSTR_NOTNULL:
 			/* processed below */
 			break;
 
@@ -2989,29 +2973,44 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
 			break;
 	}
 
-	/*
-	 * Since all other constraint types throw errors, this must be a check
-	 * constraint.  First, process the constraint expression and add an entry
-	 * to pg_constraint.
-	 */
+	if (constr->contype == CONSTR_CHECK)
+	{
+		/*
+		 * First, process the constraint expression and add an entry
+		 * to pg_constraint.
+		 */
 
-	ccbin = domainAddConstraint(domainoid, typTup->typnamespace,
-								typTup->typbasetype, typTup->typtypmod,
-								constr, NameStr(typTup->typname), constrAddr);
+		ccbin = domainAddCheckConstraint(domainoid, typTup->typnamespace,
+										 typTup->typbasetype, typTup->typtypmod,
+										 constr, NameStr(typTup->typname), constrAddr);
 
-	/*
-	 * If requested to validate the constraint, test all values stored in the
-	 * attributes based on the domain the constraint is being added to.
-	 */
-	if (!constr->skip_validation)
-		validateDomainConstraint(domainoid, ccbin);
 
-	/*
-	 * We must send out an sinval message for the domain, to ensure that any
-	 * dependent plans get rebuilt.  Since this command doesn't change the
-	 * domain's pg_type row, that won't happen automatically; do it manually.
-	 */
-	CacheInvalidateHeapTuple(typrel, tup, NULL);
+		/*
+		 * If requested to validate the constraint, test all values stored in the
+		 * attributes based on the domain the constraint is being added to.
+		 */
+		if (!constr->skip_validation)
+			validateDomainCheckConstraint(domainoid, ccbin);
+
+		/*
+		 * We must send out an sinval message for the domain, to ensure that any
+		 * dependent plans get rebuilt.  Since this command doesn't change the
+		 * domain's pg_type row, that won't happen automatically; do it manually.
+		 */
+		CacheInvalidateHeapTuple(typrel, tup, NULL);
+	}
+	else if (constr->contype == CONSTR_NOTNULL)
+	{
+		domainAddNotNullConstraint(domainoid, typTup->typnamespace,
+								   typTup->typbasetype, typTup->typtypmod,
+								   constr, NameStr(typTup->typname), constrAddr);
+
+		if (!constr->skip_validation)
+			validateDomainNotNullConstraint(domainoid);
+
+		typTup->typnotnull = true;
+		CatalogTupleUpdate(typrel, &tup->t_self, tup);
+	}
 
 	ObjectAddressSet(address, TypeRelationId, domainoid);
 
@@ -3096,7 +3095,7 @@ AlterDomainValidateConstraint(List *names, const char *constrName)
 	val = SysCacheGetAttrNotNull(CONSTROID, tuple, Anum_pg_constraint_conbin);
 	conbin = TextDatumGetCString(val);
 
-	validateDomainConstraint(domainoid, conbin);
+	validateDomainCheckConstraint(domainoid, conbin);
 
 	/*
 	 * Now update the catalog, while we have the door open.
@@ -3123,7 +3122,69 @@ AlterDomainValidateConstraint(List *names, const char *constrName)
 }
 
 static void
-validateDomainConstraint(Oid domainoid, char *ccbin)
+validateDomainNotNullConstraint(Oid domainoid)
+{
+	List	   *rels;
+	ListCell   *rt;
+
+	/* Fetch relation list with attributes based on this domain */
+	/* ShareLock is sufficient to prevent concurrent data changes */
+
+	rels = get_rels_with_domain(domainoid, ShareLock);
+
+	foreach(rt, rels)
+	{
+		RelToCheck *rtc = (RelToCheck *) lfirst(rt);
+		Relation	testrel = rtc->rel;
+		TupleDesc	tupdesc = RelationGetDescr(testrel);
+		TupleTableSlot *slot;
+		TableScanDesc scan;
+		Snapshot	snapshot;
+
+		/* Scan all tuples in this relation */
+		snapshot = RegisterSnapshot(GetLatestSnapshot());
+		scan = table_beginscan(testrel, snapshot, 0, NULL);
+		slot = table_slot_create(testrel, NULL);
+		while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
+		{
+			int			i;
+
+			/* Test attributes that are of the domain */
+			for (i = 0; i < rtc->natts; i++)
+			{
+				int			attnum = rtc->atts[i];
+				Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
+
+				if (slot_attisnull(slot, attnum))
+				{
+					/*
+					 * In principle the auxiliary information for this
+					 * error should be errdatatype(), but errtablecol()
+					 * seems considerably more useful in practice.  Since
+					 * this code only executes in an ALTER DOMAIN command,
+					 * the client should already know which domain is in
+					 * question.
+					 */
+					ereport(ERROR,
+							(errcode(ERRCODE_NOT_NULL_VIOLATION),
+							 errmsg("column \"%s\" of table \"%s\" contains null values",
+									NameStr(attr->attname),
+									RelationGetRelationName(testrel)),
+							 errtablecol(testrel, attnum)));
+				}
+			}
+		}
+		ExecDropSingleTupleTableSlot(slot);
+		table_endscan(scan);
+		UnregisterSnapshot(snapshot);
+
+		/* Close each rel after processing, but keep lock */
+		table_close(testrel, NoLock);
+	}
+}
+
+static void
+validateDomainCheckConstraint(Oid domainoid, char *ccbin)
 {
 	Expr	   *expr = (Expr *) stringToNode(ccbin);
 	List	   *rels;
@@ -3429,12 +3490,12 @@ checkDomainOwner(HeapTuple tup)
 }
 
 /*
- * domainAddConstraint - code shared between CREATE and ALTER DOMAIN
+ * domainAddCheckConstraint - code shared between CREATE and ALTER DOMAIN
  */
 static char *
-domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
-					int typMod, Constraint *constr,
-					const char *domainName, ObjectAddress *constrAddr)
+domainAddCheckConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
+						 int typMod, Constraint *constr,
+						 const char *domainName, ObjectAddress *constrAddr)
 {
 	Node	   *expr;
 	char	   *ccbin;
@@ -3442,6 +3503,8 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
 	CoerceToDomainValue *domVal;
 	Oid			ccoid;
 
+	Assert(constr->contype == CONSTR_CHECK);
+
 	/*
 	 * Assign or validate constraint name
 	 */
@@ -3561,7 +3624,7 @@ replace_domain_constraint_value(ParseState *pstate, ColumnRef *cref)
 {
 	/*
 	 * Check for a reference to "value", and if that's what it is, replace
-	 * with a CoerceToDomainValue as prepared for us by domainAddConstraint.
+	 * with a CoerceToDomainValue as prepared for us by domainAddCheckConstraint.
 	 * (We handle VALUE as a name, not a keyword, to avoid breaking a lot of
 	 * applications that have used VALUE as a column name in the past.)
 	 */
@@ -3583,6 +3646,78 @@ replace_domain_constraint_value(ParseState *pstate, ColumnRef *cref)
 	return NULL;
 }
 
+/*
+ * domainAddNotNullConstraint - code shared between CREATE and ALTER DOMAIN
+ */
+static void
+domainAddNotNullConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
+						   int typMod, Constraint *constr,
+						   const char *domainName, ObjectAddress *constrAddr)
+{
+	Oid			ccoid;
+
+	Assert(constr->contype == CONSTR_NOTNULL);
+
+	/*
+	 * Assign or validate constraint name
+	 */
+	if (constr->conname)
+	{
+		if (ConstraintNameIsUsed(CONSTRAINT_DOMAIN,
+								 domainOid,
+								 constr->conname))
+			ereport(ERROR,
+					(errcode(ERRCODE_DUPLICATE_OBJECT),
+					 errmsg("constraint \"%s\" for domain \"%s\" already exists",
+							constr->conname, domainName)));
+	}
+	else
+		constr->conname = ChooseConstraintName(domainName,
+											   NULL,
+											   "not_null",
+											   domainNamespace,
+											   NIL);
+
+	/*
+	 * Store the constraint in pg_constraint
+	 */
+	ccoid =
+		CreateConstraintEntry(constr->conname,	/* Constraint Name */
+							  domainNamespace,	/* namespace */
+							  CONSTRAINT_NOTNULL, /* Constraint Type */
+							  false,	/* Is Deferrable */
+							  false,	/* Is Deferred */
+							  !constr->skip_validation, /* Is Validated */
+							  InvalidOid,	/* no parent constraint */
+							  InvalidOid,	/* not a relation constraint */
+							  NULL,
+							  0,
+							  0,
+							  domainOid,	/* domain constraint */
+							  InvalidOid,	/* no associated index */
+							  InvalidOid,	/* Foreign key fields */
+							  NULL,
+							  NULL,
+							  NULL,
+							  NULL,
+							  0,
+							  ' ',
+							  ' ',
+							  NULL,
+							  0,
+							  ' ',
+							  NULL, /* not an exclusion constraint */
+							  NULL,
+							  NULL,
+							  true, /* is local */
+							  0,	/* inhcount */
+							  false,	/* connoinherit */
+							  false);	/* is_internal */
+
+	if (constrAddr)
+		ObjectAddressSet(*constrAddr, ConstraintRelationId, ccoid);
+}
+
 
 /*
  * Execute ALTER TYPE RENAME
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 608cd5e8e4..ee1adff932 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -1068,7 +1068,7 @@ load_domaintype_info(TypeCacheEntry *typentry)
 			Expr	   *check_expr;
 			DomainConstraintState *r;
 
-			/* Ignore non-CHECK constraints (presently, shouldn't be any) */
+			/* Ignore non-CHECK constraints */
 			if (c->contype != CONSTRAINT_CHECK)
 				continue;
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 34fd0a86e9..16e9528d6a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7552,7 +7552,7 @@ getDomainConstraints(Archive *fout, TypeInfo *tyinfo)
 							 "pg_catalog.pg_get_constraintdef(oid) AS consrc, "
 							 "convalidated "
 							 "FROM pg_catalog.pg_constraint "
-							 "WHERE contypid = $1 "
+							 "WHERE contypid = $1 AND contype = 'c' "
 							 "ORDER BY conname");
 
 		ExecuteSqlStatement(fout, query->data);
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index a026b42515..c007fe81a8 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -247,6 +247,7 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
 
 extern HeapTuple findNotNullConstraintAttnum(Oid relid, AttrNumber attnum);
 extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
+extern HeapTuple findDomainNotNullConstraint(Oid typid);
 extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
 extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
 									  bool is_no_inherit);
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index e70aebd70c..799b268ac7 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -798,6 +798,29 @@ alter domain con drop constraint nonexistent;
 ERROR:  constraint "nonexistent" of domain "con" does not exist
 alter domain con drop constraint if exists nonexistent;
 NOTICE:  constraint "nonexistent" of domain "con" does not exist, skipping
+-- not-null constraints
+create domain connotnull integer;
+create table domconnotnulltest
+( col1 connotnull
+, col2 connotnull
+);
+insert into domconnotnulltest default values;
+alter domain connotnull add not null value; -- fails
+ERROR:  column "col1" of table "domconnotnulltest" contains null values
+update domconnotnulltest set col1 = 5;
+alter domain connotnull add not null value; -- fails
+ERROR:  column "col2" of table "domconnotnulltest" contains null values
+update domconnotnulltest set col2 = 6;
+alter domain connotnull add constraint constr1 not null value;
+update domconnotnulltest set col1 = null; -- fails
+ERROR:  domain connotnull does not allow null values
+alter domain connotnull drop constraint constr1;
+update domconnotnulltest set col1 = null;
+drop domain connotnull cascade;
+NOTICE:  drop cascades to 2 other objects
+DETAIL:  drop cascades to column col2 of table domconnotnulltest
+drop cascades to column col1 of table domconnotnulltest
+drop table domconnotnulltest;
 -- Test ALTER DOMAIN .. CONSTRAINT .. NOT VALID
 create domain things AS INT;
 CREATE TABLE thethings (stuff things);
@@ -1223,12 +1246,13 @@ SELECT * FROM information_schema.column_domain_usage
 SELECT * FROM information_schema.domain_constraints
   WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
   ORDER BY constraint_name;
- constraint_catalog | constraint_schema | constraint_name | domain_catalog | domain_schema | domain_name | is_deferrable | initially_deferred 
---------------------+-------------------+-----------------+----------------+---------------+-------------+---------------+--------------------
- regression         | public            | con_check       | regression     | public        | con         | NO            | NO
- regression         | public            | meow            | regression     | public        | things      | NO            | NO
- regression         | public            | pos_int_check   | regression     | public        | pos_int     | NO            | NO
-(3 rows)
+ constraint_catalog | constraint_schema | constraint_name  | domain_catalog | domain_schema | domain_name | is_deferrable | initially_deferred 
+--------------------+-------------------+------------------+----------------+---------------+-------------+---------------+--------------------
+ regression         | public            | con_check        | regression     | public        | con         | NO            | NO
+ regression         | public            | meow             | regression     | public        | things      | NO            | NO
+ regression         | public            | pos_int_check    | regression     | public        | pos_int     | NO            | NO
+ regression         | public            | pos_int_not_null | regression     | public        | pos_int     | NO            | NO
+(4 rows)
 
 SELECT * FROM information_schema.domains
   WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
@@ -1247,10 +1271,11 @@ SELECT * FROM information_schema.check_constraints
             FROM information_schema.domain_constraints
             WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
   ORDER BY constraint_name;
- constraint_catalog | constraint_schema | constraint_name | check_clause 
---------------------+-------------------+-----------------+--------------
- regression         | public            | con_check       | (VALUE > 0)
- regression         | public            | meow            | (VALUE < 11)
- regression         | public            | pos_int_check   | (VALUE > 0)
-(3 rows)
+ constraint_catalog | constraint_schema | constraint_name  |   check_clause    
+--------------------+-------------------+------------------+-------------------
+ regression         | public            | con_check        | (VALUE > 0)
+ regression         | public            | meow             | (VALUE < 11)
+ regression         | public            | pos_int_check    | (VALUE > 0)
+ regression         | public            | pos_int_not_null | VALUE IS NOT NULL
+(4 rows)
 
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 813048c19f..0e71f04004 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -469,6 +469,32 @@
 alter domain con drop constraint nonexistent;
 alter domain con drop constraint if exists nonexistent;
 
+-- not-null constraints
+create domain connotnull integer;
+create table domconnotnulltest
+( col1 connotnull
+, col2 connotnull
+);
+
+insert into domconnotnulltest default values;
+alter domain connotnull add not null value; -- fails
+
+update domconnotnulltest set col1 = 5;
+alter domain connotnull add not null value; -- fails
+
+update domconnotnulltest set col2 = 6;
+
+alter domain connotnull add constraint constr1 not null value;
+
+update domconnotnulltest set col1 = null; -- fails
+
+alter domain connotnull drop constraint constr1;
+
+update domconnotnulltest set col1 = null;
+
+drop domain connotnull cascade;
+drop table domconnotnulltest;
+
 -- Test ALTER DOMAIN .. CONSTRAINT .. NOT VALID
 create domain things AS INT;
 CREATE TABLE thethings (stuff things);
-- 
2.42.1



^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Catalog domain not-null constraints
  2023-11-23 06:56 Catalog domain not-null constraints Peter Eisentraut <[email protected]>
@ 2023-11-23 13:13 ` Aleksander Alekseev <[email protected]>
  2023-11-23 16:35   ` Re: Catalog domain not-null constraints Alvaro Herrera <[email protected]>
  1 sibling, 1 reply; 5+ messages in thread

From: Aleksander Alekseev @ 2023-11-23 13:13 UTC (permalink / raw)
  To: pgsql-hackers; +Cc: Peter Eisentraut <[email protected]>

Hi,

> This patch set applies the explicit catalog representation of not-null
> constraints introduced by b0e96f3119 for table constraints also to
> domain not-null constraints.

Interestingly enough according to the documentation this syntax is
already supported [1][2], but the actual query will fail on `master`:

```
=# create domain connotnull integer;
CREATE DOMAIN
=# alter domain connotnull add not null value;
ERROR:  unrecognized constraint subtype: 1
```

I wonder if we should reflect this limitation in the documentation
and/or show better error messages. This could be quite surprising to
the user. However if we change the documentation on the `master`
branch this patch will have to change it back.

I was curious about the semantic difference between `SET NOT NULL` and
`ADD NOT NULL value`. When I wanted to figure this out I discovered
something that seems to be a bug:

```
=# create domain connotnull1 integer;
=# create domain connotnull2 integer;
=# alter domain connotnull1 add not null value;
=# alter domain connotnull2 set not null;
=# \dD
ERROR:  unexpected null value in cached tuple for catalog
pg_constraint column conkey
```

Also it turned out that I can do both: `SET NOT NULL` and `ADD NOT
NULL value` for the same domain. Is it an intended behavior? We should
either forbid it or cover this case with a test.

NOT VALID is not supported:

```
=# alter domain connotnull add not null value not valid;
ERROR:  NOT NULL constraints cannot be marked NOT VALID
```

... and this is correct: "NOT VALID is only accepted for CHECK
constraints" [1]. This code path however doesn't seem to be
test-covered even on `master`. While on it, I suggest fixing this.

[1]: https://www.postgresql.org/docs/current/sql-alterdomain.html
[2]: https://www.postgresql.org/docs/current/sql-createdomain.html

-- 
Best regards,
Aleksander Alekseev

^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Catalog domain not-null constraints
  2023-11-23 06:56 Catalog domain not-null constraints Peter Eisentraut <[email protected]>
  2023-11-23 13:13 ` Re: Catalog domain not-null constraints Aleksander Alekseev <[email protected]>
@ 2023-11-23 16:35   ` Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Alvaro Herrera @ 2023-11-23 16:35 UTC (permalink / raw)
  To: Aleksander Alekseev <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]>

On 2023-Nov-23, Aleksander Alekseev wrote:

> Interestingly enough according to the documentation this syntax is
> already supported [1][2], but the actual query will fail on `master`:
> 
> ```
> =# create domain connotnull integer;
> CREATE DOMAIN
> =# alter domain connotnull add not null value;
> ERROR:  unrecognized constraint subtype: 1
> ```

Hah, nice ... this only fails in this way on master, though, as a
side-effect of previous NOT NULL work during this cycle.  So if we take
Peter's patch, we don't need to worry about it.  In 16 it behaves
properly, with a normal syntax error.

> ```
> =# create domain connotnull1 integer;
> =# create domain connotnull2 integer;
> =# alter domain connotnull1 add not null value;
> =# alter domain connotnull2 set not null;
> =# \dD
> ERROR:  unexpected null value in cached tuple for catalog
> pg_constraint column conkey
> ```

This is also a master-only problem, as "add not null" is rejected in 16
with a syntax error (and obviously \dD doesn't fail).

> NOT VALID is not supported:
> 
> ```
> =# alter domain connotnull add not null value not valid;
> ERROR:  NOT NULL constraints cannot be marked NOT VALID
> ```

Yeah, it'll take more work to let NOT NULL constraints be marked NOT
VALID, both on domains and on tables.  It'll be a good feature for sure.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La victoria es para quien se atreve a estar solo"






^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Catalog domain not-null constraints
  2023-11-23 06:56 Catalog domain not-null constraints Peter Eisentraut <[email protected]>
@ 2023-11-23 16:38 ` Alvaro Herrera <[email protected]>
  1 sibling, 0 replies; 5+ messages in thread

From: Alvaro Herrera @ 2023-11-23 16:38 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; +Cc: pgsql-hackers

On 2023-Nov-23, Peter Eisentraut wrote:

> This patch set applies the explicit catalog representation of not-null
> constraints introduced by b0e96f3119 for table constraints also to domain
> not-null constraints.

I like the idea of having domain not-null constraints appear in
pg_constraint.

> Since there is no inheritance or primary keys etc., this is much simpler and
> just applies the existing infrastructure to domains as well.

If you create a table with column of domain that has a NOT NULL
constraint, what happens?  I mean, is the table column marked
attnotnull, and how does it behave?  Is there a separate pg_constraint
row for the constraint in the table?  What happens if you do
ALTER TABLE ... DROP NOT NULL for that column?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Por suerte hoy explotó el califont porque si no me habría muerto
 de aburrido"  (Papelucho)

^ permalink  raw  reply  [nested|flat] 5+ messages in thread

end of thread, other threads:[~2023-11-23 16:38 UTC | newest]

Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 15:26 [PATCH 2/8] Move IS [NOT] NULL handling from BRIN support functions Tomas Vondra <[email protected]>
2023-11-23 06:56 Catalog domain not-null constraints Peter Eisentraut <[email protected]>
2023-11-23 13:13 ` Re: Catalog domain not-null constraints Aleksander Alekseev <[email protected]>
2023-11-23 16:35   ` Re: Catalog domain not-null constraints Alvaro Herrera <[email protected]>
2023-11-23 16:38 ` Re: Catalog domain not-null constraints Alvaro Herrera <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox