On Wed, Dec 4, 2024 at 10:52 AM jian he <jian.universal...@gmail.com> wrote:
>
> hi.
>
> heap_create_with_catalog argument (cooked_constraints):
> passed as NIL in function {create_toast_table, make_new_heap}
> passed as list_concat(cookedDefaults,old_constraints) in DefineRelation
>
> in DefineRelation we have function call:
> MergeAttributes
> heap_create_with_catalog
> StoreConstraints
>
> StoreConstraints second argument: cooked_constraints, some is comes from
> DefineRelation->MergeAttributes old_constraints:
> {
> stmt->tableElts = MergeAttributes(stmt->tableElts, inheritOids,
> stmt->relation->relpersistence, stmt->partbound != NULL, &old_constraints,
> &old_notnulls);
> }
>
> My understanding from DefineRelation->MergeAttributes is that old_constraints
> will only have CHECK constraints.
> that means heap_create_with_catalog->StoreConstraints
> StoreConstraints didn't actually handle CONSTR_NOTNULL.
>
> heap_create_with_catalog comments also says:
> * cooked_constraints: list of precooked check constraints and defaults
>
> coverage https://coverage.postgresql.org/src/backend/catalog/heap.c.gcov.html
> also shows StoreConstraints, CONSTR_NOTNULL never being called,
> which is added by this thread.
>
>
> my question is can we remove StoreConstraints, CONSTR_NOTNULL handling.
> we have 3 functions {StoreConstraints, AddRelationNotNullConstraints,
> AddRelationNewConstraints} that will call StoreRelNotNull to store the
> not-null
> constraint. That means if we want to bullet proof that something is
> conflicting
> with not-null, we need to add code to check all these 3 places.
> removing StoreConstraints handling not-null seems helpful.
>
>
> also comments in MergeAttributes:
> * Output arguments:
> * 'supconstr' receives a list of constraints belonging to the parents,
> * updated as necessary to be valid for the child.
> * 'supnotnulls' receives a list of CookedConstraints that corresponds to
> * constraints coming from inheritance parents.
>
> can we be explicit that "supconstr" is only about CHECK constraint,
> "supnotnulls" is
> only about NOT-NULL constraint.
patch attached.
also change comments of heap_create_with_catalog,
StoreConstraints, MergeAttributes.
so we can clear idea what's kind of constraints we are dealing with
in these functions.
From 1a2d75b6d107eb372edfca8e9a2e7df19ba08a6e Mon Sep 17 00:00:00 2001
From: jian he <jian.universal...@gmail.com>
Date: Thu, 12 Dec 2024 10:45:58 +0800
Subject: [PATCH v1 1/1] remove StoreConstraints dealing with CONSTR_NOTNULL
StoreConstraints never need to deal with CONSTR_NOTNULL.
so remove that part.
because of this, change comments for functions: heap_create_with_catalog,
StoreConstraints, MergeAttributes.
so we can clear idea what's kind of constraints we are dealing with
in these functions.
discussion: https://postgr.es/m/CACJufxFxzqrCiUNfjJ0tQU+=nKQkQCGtGzUBude=smowj5v...@mail.gmail.com
---
src/backend/catalog/heap.c | 11 ++---------
src/backend/commands/tablecmds.c | 8 ++++----
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index d7b88b61dc..6a6c328a27 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1489,7 +1489,7 @@ heap_create_with_catalog(const char *relname,
InvokeObjectPostCreateHookArg(RelationRelationId, relid, 0, is_internal);
/*
- * Store any supplied constraints and defaults.
+ * Store any supplied CHECK constraints and defaults.
*
* NB: this may do a CommandCounterIncrement and rebuild the relcache
* entry, so the relation must be valid and self-consistent at this point.
@@ -2224,7 +2224,7 @@ StoreRelNotNull(Relation rel, const char *nnname, AttrNumber attnum,
}
/*
- * Store defaults and constraints (passed as a list of CookedConstraint).
+ * Store defaults and CHECK constraints (passed as a list of CookedConstraint).
*
* Each CookedConstraint struct is modified to store the new catalog tuple OID.
*
@@ -2268,13 +2268,6 @@ StoreConstraints(Relation rel, List *cooked_constraints, bool is_internal)
numchecks++;
break;
- case CONSTR_NOTNULL:
- con->conoid =
- StoreRelNotNull(rel, con->name, con->attnum,
- !con->skip_validation, con->is_local,
- con->inhcount, con->is_no_inherit);
- break;
-
default:
elog(ERROR, "unrecognized constraint type: %d",
(int) con->contype);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6ccae4cb4a..9142f8e4ad 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2433,10 +2433,10 @@ storage_name(char c)
* 'is_partition' tells if the table is a partition.
*
* Output arguments:
- * 'supconstr' receives a list of constraints belonging to the parents,
- * updated as necessary to be valid for the child.
- * 'supnotnulls' receives a list of CookedConstraints that corresponds to
- * constraints coming from inheritance parents.
+ * 'supconstr' receives a list of CookedConstraints CHECK constraints,
+ * belonging to the parents updated as necessary to be valid for the child.
+ * 'supnotnulls' receives a list of CookedConstraints NOT NULL constraints
+ * that corresponds to NOT NULL constraints coming from inheritance parents.
*
* Return value:
* Completed schema list.
--
2.34.1
