public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v6 1/3] Rework deleteObjectsInList to allow objtype-specific checks 6+ messages / 4 participants [nested] [flat]
* [PATCH v6 1/3] Rework deleteObjectsInList to allow objtype-specific checks @ 2018-11-28 14:52 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Alvaro Herrera @ 2018-11-28 14:52 UTC (permalink / raw) This doesn't change any functionality yet. --- src/backend/catalog/dependency.c | 41 +++++++++++++++++++------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 2048d71535b..0b4c47b808c 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -230,29 +230,38 @@ deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel, int i; /* - * Keep track of objects for event triggers, if necessary. + * Invoke pre-deletion callbacks and keep track of objects for event + * triggers, if necessary. */ - if (trackDroppedObjectsNeeded() && !(flags & PERFORM_DELETION_INTERNAL)) + for (i = 0; i < targetObjects->numrefs; i++) { - for (i = 0; i < targetObjects->numrefs; i++) + const ObjectAddress *thisobj = &targetObjects->refs[i]; + Oid objectClass = getObjectClass(thisobj); + + if (trackDroppedObjectsNeeded() && !(flags & PERFORM_DELETION_INTERNAL)) { - const ObjectAddress *thisobj = &targetObjects->refs[i]; - const ObjectAddressExtra *extra = &targetObjects->extras[i]; - bool original = false; - bool normal = false; - - if (extra->flags & DEPFLAG_ORIGINAL) - original = true; - if (extra->flags & DEPFLAG_NORMAL) - normal = true; - if (extra->flags & DEPFLAG_REVERSE) - normal = true; - - if (EventTriggerSupportsObjectClass(getObjectClass(thisobj))) + if (EventTriggerSupportsObjectClass(objectClass)) { + bool original = false; + bool normal = false; + const ObjectAddressExtra *extra = &targetObjects->extras[i]; + + if (extra->flags & DEPFLAG_ORIGINAL) + original = true; + if (extra->flags & DEPFLAG_NORMAL || + extra->flags & DEPFLAG_REVERSE) + normal = true; + EventTriggerSQLDropAddObject(thisobj, original, normal); } } + + /* Invoke class-specific pre-deletion checks */ + switch (objectClass) + { + default: + break; + } } /* -- 2.17.1 --n8g4imXOkfNTN/H1 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0002-Add-index_get_partition-convenience-function.patch" ^ permalink raw reply [nested|flat] 6+ messages in thread
* [PATCH v21 2/8] Row pattern recognition patch (parse/analysis). @ 2024-08-26 04:32 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Tatsuo Ishii @ 2024-08-26 04:32 UTC (permalink / raw) --- src/backend/parser/parse_agg.c | 7 + src/backend/parser/parse_clause.c | 296 +++++++++++++++++++++++++++++- src/backend/parser/parse_expr.c | 6 + src/backend/parser/parse_func.c | 3 + 4 files changed, 311 insertions(+), 1 deletion(-) diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index bee7d8346a..9bc22a836a 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -577,6 +577,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr) errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; + /* * There is intentionally no default: case here, so that the * compiler will warn if we add a new ParseExprKind without @@ -967,6 +971,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 8118036495..9762dce81f 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -98,7 +98,14 @@ static WindowClause *findWindowClause(List *wclist, const char *name); static Node *transformFrameOffset(ParseState *pstate, int frameOptions, Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc, Node *clause); - +static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, + List **targetlist); +static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, + List **targetlist); +static void transformPatternClause(ParseState *pstate, WindowClause *wc, + WindowDef *windef); +static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, + WindowDef *windef); /* * transformFromClause - @@ -2956,6 +2963,10 @@ transformWindowDefinitions(ParseState *pstate, rangeopfamily, rangeopcintype, &wc->endInRangeFunc, windef->endOffset); + + /* Process Row Pattern Recognition related clauses */ + transformRPR(pstate, wc, windef, targetlist); + wc->winref = winref; result = lappend(result, wc); @@ -3820,3 +3831,286 @@ transformFrameOffset(ParseState *pstate, int frameOptions, return node; } + +/* + * transformRPR + * Process Row Pattern Recognition related clauses + */ +static void +transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, + List **targetlist) +{ + /* + * Window definition exists? + */ + if (windef == NULL) + return; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + /* Check Frame option. Frame must start at current row */ + if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("FRAME must start at current row when row patttern recognition is used"))); + + /* Transform AFTER MACH SKIP TO clause */ + wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo; + + /* Transform AFTER MACH SKIP TO variable */ + wc->rpSkipVariable = windef->rpCommonSyntax->rpSkipVariable; + + /* Transform SEEK or INITIAL clause */ + wc->initial = windef->rpCommonSyntax->initial; + + /* Transform DEFINE clause into list of TargetEntry's */ + wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist); + + /* Check PATTERN clause and copy to patternClause */ + transformPatternClause(pstate, wc, windef); + + /* Transform MEASURE clause */ + transformMeasureClause(pstate, wc, windef); +} + +/* + * transformDefineClause Process DEFINE clause and transform ResTarget into + * list of TargetEntry. + * + * XXX we only support column reference in row pattern definition search + * condition, e.g. "price". <row pattern definition variable name>.<column + * reference> is not supported, e.g. "A.price". + */ +static List * +transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, + List **targetlist) +{ + /* DEFINE variable name initials */ + static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz"; + + ListCell *lc, + *l; + ResTarget *restarget, + *r; + List *restargets; + List *defineClause; + char *name; + int initialLen; + int i; + + /* + * If Row Definition Common Syntax exists, DEFINE clause must exist. (the + * raw parser should have already checked it.) + */ + Assert(windef->rpCommonSyntax->rpDefs != NULL); + + /* + * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE + * per the SQL standard. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + bool found = false; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *) lfirst(lc); + name = strVal(a->lexpr); + + foreach(l, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *) lfirst(l); + + if (!strcmp(restarget->name, name)) + { + found = true; + break; + } + } + + if (!found) + { + /* + * "name" is missing. So create "name AS name IS TRUE" ResTarget + * node and add it to the temporary list. + */ + A_Const *n; + + restarget = makeNode(ResTarget); + n = makeNode(A_Const); + n->val.boolval.type = T_Boolean; + n->val.boolval.boolval = true; + n->location = -1; + restarget->name = pstrdup(name); + restarget->indirection = NIL; + restarget->val = (Node *) n; + restarget->location = -1; + restargets = lappend((List *) restargets, restarget); + } + } + + if (list_length(restargets) >= 1) + { + /* add missing DEFINEs */ + windef->rpCommonSyntax->rpDefs = + list_concat(windef->rpCommonSyntax->rpDefs, restargets); + list_free(restargets); + } + + /* + * Check for duplicate row pattern definition variables. The standard + * requires that no two row pattern definition variable names shall be + * equivalent. + */ + restargets = NIL; + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + restarget = (ResTarget *) lfirst(lc); + name = restarget->name; + + /* + * Add DEFINE expression (Restarget->val) to the targetlist as a + * TargetEntry if it does not exist yet. Planner will add the column + * ref var node to the outer plan's target list later on. This makes + * DEFINE expression could access the outer tuple while evaluating + * PATTERN. + * + * XXX: adding whole expressions of DEFINE to the plan.targetlist is + * not so good, because it's not necessary to evalute the expression + * in the target list while running the plan. We should extract the + * var nodes only then add them to the plan.targetlist. + */ + findTargetlistEntrySQL99(pstate, (Node *) restarget->val, + targetlist, EXPR_KIND_RPR_DEFINE); + + /* + * Make sure that the row pattern definition search condition is a + * boolean expression. + */ + transformWhereClause(pstate, restarget->val, + EXPR_KIND_RPR_DEFINE, "DEFINE"); + + foreach(l, restargets) + { + char *n; + + r = (ResTarget *) lfirst(l); + n = r->name; + + if (!strcmp(n, name)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause", + name), + parser_errposition(pstate, exprLocation((Node *) r)))); + } + restargets = lappend(restargets, restarget); + } + list_free(restargets); + + /* + * Create list of row pattern DEFINE variable name's initial. We assign + * [a-z] to them (up to 26 variable names are allowed). + */ + restargets = NIL; + i = 0; + initialLen = strlen(defineVariableInitials); + + foreach(lc, windef->rpCommonSyntax->rpDefs) + { + char initial[2]; + + restarget = (ResTarget *) lfirst(lc); + name = restarget->name; + + if (i >= initialLen) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("number of row pattern definition variable names exceeds %d", + initialLen), + parser_errposition(pstate, + exprLocation((Node *) restarget)))); + } + initial[0] = defineVariableInitials[i++]; + initial[1] = '\0'; + wc->defineInitial = lappend(wc->defineInitial, + makeString(pstrdup(initial))); + } + + defineClause = transformTargetList(pstate, windef->rpCommonSyntax->rpDefs, + EXPR_KIND_RPR_DEFINE); + + /* mark column origins */ + markTargetListOrigins(pstate, defineClause); + + /* mark all nodes in the DEFINE clause tree with collation information */ + assign_expr_collations(pstate, (Node *) defineClause); + + return defineClause; +} + +/* + * transformPatternClause + * Process PATTERN clause and return PATTERN clause in the raw parse tree + */ +static void +transformPatternClause(ParseState *pstate, WindowClause *wc, + WindowDef *windef) +{ + ListCell *lc; + + /* + * Row Pattern Common Syntax clause exists? + */ + if (windef->rpCommonSyntax == NULL) + return; + + wc->patternVariable = NIL; + wc->patternRegexp = NIL; + foreach(lc, windef->rpCommonSyntax->rpPatterns) + { + A_Expr *a; + char *name; + char *regexp; + + if (!IsA(lfirst(lc), A_Expr)) + ereport(ERROR, + errmsg("node type is not A_Expr")); + + a = (A_Expr *) lfirst(lc); + name = strVal(a->lexpr); + + wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name))); + regexp = strVal(lfirst(list_head(a->name))); + + wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp))); + } +} + +/* + * transformMeasureClause + * Process MEASURE clause + * XXX MEASURE clause is not supported yet + */ +static List * +transformMeasureClause(ParseState *pstate, WindowClause *wc, + WindowDef *windef) +{ + if (windef->rowPatternMeasures == NIL) + return NIL; + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("%s", "MEASURE clause is not supported yet"), + parser_errposition(pstate, exprLocation((Node *) windef->rowPatternMeasures)))); + return NIL; +} diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 56e413da9f..c187b3278d 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -577,6 +577,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) case EXPR_KIND_COPY_WHERE: case EXPR_KIND_GENERATED_COLUMN: case EXPR_KIND_CYCLE_MARK: + case EXPR_KIND_RPR_DEFINE: /* okay */ break; @@ -1860,6 +1861,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink) case EXPR_KIND_GENERATED_COLUMN: err = _("cannot use subquery in column generation expression"); break; + case EXPR_KIND_RPR_DEFINE: + err = _("cannot use subquery in DEFINE expression"); + break; /* * There is intentionally no default: case here, so that the @@ -3199,6 +3203,8 @@ ParseExprKindName(ParseExprKind exprKind) return "GENERATED AS"; case EXPR_KIND_CYCLE_MARK: return "CYCLE"; + case EXPR_KIND_RPR_DEFINE: + return "DEFINE"; /* * There is intentionally no default: case here, so that the diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index 9b23344a3b..4c482abb30 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2658,6 +2658,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location) case EXPR_KIND_CYCLE_MARK: errkind = true; break; + case EXPR_KIND_RPR_DEFINE: + errkind = true; + break; /* * There is intentionally no default: case here, so that the -- 2.25.1 ----Next_Part(Mon_Aug_26_13_39_47_2024_878)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v21-0003-Row-pattern-recognition-patch-rewriter.patch" ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Allow NOT VALID foreign key constraints on partitioned tables. @ 2025-01-06 04:23 Amul Sul <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Amul Sul @ 2025-01-06 04:23 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: [email protected] <[email protected]> On Fri, Jan 3, 2025 at 12:11 AM Álvaro Herrera <[email protected]> wrote: > > > > On Thu, Jan 2, 2025, at 5:49 PM, Amul Sul wrote: > > When adding a new FK constraint or attaching a partitioned table, where > matching FK constraints are merged, we allow the parent constraint to be NOT > VALID while the child constraint remains VALID, which is harmless. However, the > reverse scenario -- where the parent constraint is VALID and the child is NOT > VALID -- is incorrect. To address this, when merging a NOT VALID FK constraint > from the child with a VALID parent constraint, it implicitly validates the > child constraint against its existing data and marks it as VALID. This behavior > aligns with adding a new FK constraint directly to the child table, which would > also validate the existing data. > > > Hmm, I'm not sure about this, which may cause surprising delays. Maybe it would be better that the operation fails with an error, so that the user can do VALIDATE CONSTRAINT explicitly and retry the ATTACH once all the partitions have been so processed. Error reporting would have made this straightforward, but the delay is not a new, and the patch does not introduce any additional delay. Setting the patch aside for a moment, consider the current behavior on the master branch: if you have a partitioned table(see e.g. below) where one of the child tables has a NOT VALID foreign key constraint, adding a new VALID foreign key constraint to the partitioned table will ignore the existing constraint on the child table. Instead, it creates a new constraint, which ultimately triggers a scan of the child table to validate the new constraint. E.g. create table bar(i int primary key); create table foo(i int) partition by range(i); create table foo_p1 partition of foo for values from (0) to (10); insert into foo_p1 values(1); -- value doesn't exists in bar alter table foo_p1 add constraint fk_con foreign key(i) references bar NOT VALID; -- ok -- following triggers the validation and fails. alter table foo add constraint fk_con foreign key(i) references bar; The behavior with the patch remains the same, but instead of creating a new foreign key constraint, it merges with the existing one and validates it. Regards, Amul ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Allow NOT VALID foreign key constraints on partitioned tables. @ 2025-01-15 05:34 Amul Sul <[email protected]> parent: Amul Sul <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Amul Sul @ 2025-01-15 05:34 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: [email protected] <[email protected]> On Mon, Jan 6, 2025 at 9:53 AM Amul Sul <[email protected]> wrote: > I made the minor changes to the attached version and rebased it against the latest master(9a45a89c38f). Regards, Amul Attachments: [application/octet-stream] v2-0001-Refactor-Split-ATExecValidateConstraint.patch (10.9K, ../../CAAJ_b95pjru9JxU4Ve8yRFSCPO8VNmur8jfuHrACWuTBDt97zg@mail.gmail.com/2-v2-0001-Refactor-Split-ATExecValidateConstraint.patch) download | inline diff: From da557aa6b26c40e50ca517cd598c68074f9db22b Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 2 Jan 2025 17:15:30 +0530 Subject: [PATCH v2 1/2] Refactor: Split ATExecValidateConstraint() Splitting ATExecValidateConstraint() into two separate functions to handle FOREIGN KEY and CHECK constraints respectively. The next patch requires the FOREIGN KEY validation code in a separate function so it can be called directly. Additionally, the function needs to be recursive to handle NOT VALID constraints on declarative partitions. Although moving the CHECK constraint-related code is not strictly necessary for this task, maintaining separate code for FOREIGN KEY constraints makes it logical to do the same for CHECK constraints. This separation will also simplify adding support for other constraints (e.g., NOT NULL) in ATExecValidateConstraint() in the future. --- src/backend/commands/tablecmds.c | 282 +++++++++++++++++++------------ 1 file changed, 171 insertions(+), 111 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4fc54bd6eba..c0881832eb1 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -394,6 +394,11 @@ static ObjectAddress ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, static bool ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, Relation rel, HeapTuple contuple, List **otherrelids, LOCKMODE lockmode); +static void QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, + HeapTuple contuple, LOCKMODE lockmode); +static void QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relation rel, + char *constrName, HeapTuple contuple, + bool recurse, bool recursing, LOCKMODE lockmode); static ObjectAddress ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, bool recurse, bool recursing, LOCKMODE lockmode); @@ -11973,6 +11978,169 @@ ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, return changed; } +/* + * QueueFKConstraintValidation + * + * Add an entry to the wqueue to validate the given foreign key constraint in + * Phase 3 and update the convalidated field in the pg_constraint catalog + * for the specified relation. + */ +static void +QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, + HeapTuple contuple, LOCKMODE lockmode) +{ + Form_pg_constraint con; + AlteredTableInfo *tab; + HeapTuple copyTuple; + Form_pg_constraint copy_con; + + con = (Form_pg_constraint) GETSTRUCT(contuple); + Assert(con->contype == CONSTRAINT_FOREIGN); + + if (rel->rd_rel->relkind == RELKIND_RELATION) + { + NewConstraint *newcon; + Constraint *fkconstraint; + + /* Queue validation for phase 3 */ + fkconstraint = makeNode(Constraint); + /* for now this is all we need */ + fkconstraint->conname = pstrdup(NameStr(con->conname)); + + newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); + newcon->name = fkconstraint->conname; + newcon->contype = CONSTR_FOREIGN; + newcon->refrelid = con->confrelid; + newcon->refindid = con->conindid; + newcon->conid = con->oid; + newcon->qual = (Node *) fkconstraint; + + /* Find or create work queue entry for this table */ + tab = ATGetQueueEntry(wqueue, rel); + tab->constraints = lappend(tab->constraints, newcon); + } + + /* + * We disallow creating invalid foreign keys to or from partitioned + * tables, so ignoring the recursion bit is okay. + */ + + /* + * Now update the catalog, while we have the door open. + */ + copyTuple = heap_copytuple(contuple); + copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple); + copy_con->convalidated = true; + CatalogTupleUpdate(conrel, ©Tuple->t_self, copyTuple); + + InvokeObjectPostAlterHook(ConstraintRelationId, con->oid, 0); + + heap_freetuple(copyTuple); +} + +/* + * QueueCheckConstraintValidation + * + * Add an entry to the wqueue to validate the given check constraint in Phase 3 + * and update the convalidated field in the pg_constraint catalog for the + * specified relation and all its inheriting children. + */ +static void +QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relation rel, + char *constrName, HeapTuple contuple, + bool recurse, bool recursing, LOCKMODE lockmode) +{ + Form_pg_constraint con; + AlteredTableInfo *tab; + HeapTuple copyTuple; + Form_pg_constraint copy_con; + + List *children = NIL; + ListCell *child; + NewConstraint *newcon; + Datum val; + char *conbin; + + con = (Form_pg_constraint) GETSTRUCT(contuple); + Assert(con->contype == CONSTRAINT_CHECK); + + /* + * If we're recursing, the parent has already done this, so skip it. + * Also, if the constraint is a NO INHERIT constraint, we shouldn't try to + * look for it in the children. + */ + if (!recursing && !con->connoinherit) + children = find_all_inheritors(RelationGetRelid(rel), + lockmode, NULL); + + /* + * For CHECK constraints, we must ensure that we only mark the constraint + * as validated on the parent if it's already validated on the children. + * + * We recurse before validating on the parent, to reduce risk of + * deadlocks. + */ + foreach(child, children) + { + Oid childoid = lfirst_oid(child); + Relation childrel; + + if (childoid == RelationGetRelid(rel)) + continue; + + /* + * If we are told not to recurse, there had better not be any child + * tables, because we can't mark the constraint on the parent valid + * unless it is valid for all child tables. + */ + if (!recurse) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("constraint must be validated on child tables too"))); + + /* find_all_inheritors already got lock */ + childrel = table_open(childoid, NoLock); + + ATExecValidateConstraint(wqueue, childrel, constrName, false, + true, lockmode); + table_close(childrel, NoLock); + } + + /* Queue validation for phase 3 */ + newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); + newcon->name = constrName; + newcon->contype = CONSTR_CHECK; + newcon->refrelid = InvalidOid; + newcon->refindid = InvalidOid; + newcon->conid = con->oid; + + val = SysCacheGetAttrNotNull(CONSTROID, contuple, + Anum_pg_constraint_conbin); + conbin = TextDatumGetCString(val); + newcon->qual = (Node *) stringToNode(conbin); + + /* Find or create work queue entry for this table */ + tab = ATGetQueueEntry(wqueue, rel); + tab->constraints = lappend(tab->constraints, newcon); + + /* + * Invalidate relcache so that others see the new validated constraint. + */ + CacheInvalidateRelcache(rel); + + /* + * Now update the catalog, while we have the door open. + */ + copyTuple = heap_copytuple(contuple); + copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple); + copy_con->convalidated = true; + CatalogTupleUpdate(conrel, ©Tuple->t_self, copyTuple); + + InvokeObjectPostAlterHook(ConstraintRelationId, con->oid, 0); + + heap_freetuple(copyTuple); +} + /* * ALTER TABLE VALIDATE CONSTRAINT * @@ -12037,124 +12205,16 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, if (!con->convalidated) { - AlteredTableInfo *tab; - HeapTuple copyTuple; - Form_pg_constraint copy_con; - if (con->contype == CONSTRAINT_FOREIGN) { - NewConstraint *newcon; - Constraint *fkconstraint; - - /* Queue validation for phase 3 */ - fkconstraint = makeNode(Constraint); - /* for now this is all we need */ - fkconstraint->conname = constrName; - - newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); - newcon->name = constrName; - newcon->contype = CONSTR_FOREIGN; - newcon->refrelid = con->confrelid; - newcon->refindid = con->conindid; - newcon->conid = con->oid; - newcon->qual = (Node *) fkconstraint; - - /* Find or create work queue entry for this table */ - tab = ATGetQueueEntry(wqueue, rel); - tab->constraints = lappend(tab->constraints, newcon); - - /* - * We disallow creating invalid foreign keys to or from - * partitioned tables, so ignoring the recursion bit is okay. - */ + QueueFKConstraintValidation(wqueue, conrel, rel, tuple, lockmode); } else if (con->contype == CONSTRAINT_CHECK) { - List *children = NIL; - ListCell *child; - NewConstraint *newcon; - Datum val; - char *conbin; - - /* - * If we're recursing, the parent has already done this, so skip - * it. Also, if the constraint is a NO INHERIT constraint, we - * shouldn't try to look for it in the children. - */ - if (!recursing && !con->connoinherit) - children = find_all_inheritors(RelationGetRelid(rel), - lockmode, NULL); - - /* - * For CHECK constraints, we must ensure that we only mark the - * constraint as validated on the parent if it's already validated - * on the children. - * - * We recurse before validating on the parent, to reduce risk of - * deadlocks. - */ - foreach(child, children) - { - Oid childoid = lfirst_oid(child); - Relation childrel; - - if (childoid == RelationGetRelid(rel)) - continue; - - /* - * If we are told not to recurse, there had better not be any - * child tables, because we can't mark the constraint on the - * parent valid unless it is valid for all child tables. - */ - if (!recurse) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("constraint must be validated on child tables too"))); - - /* find_all_inheritors already got lock */ - childrel = table_open(childoid, NoLock); - - ATExecValidateConstraint(wqueue, childrel, constrName, false, - true, lockmode); - table_close(childrel, NoLock); - } - - /* Queue validation for phase 3 */ - newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); - newcon->name = constrName; - newcon->contype = CONSTR_CHECK; - newcon->refrelid = InvalidOid; - newcon->refindid = InvalidOid; - newcon->conid = con->oid; - - val = SysCacheGetAttrNotNull(CONSTROID, tuple, - Anum_pg_constraint_conbin); - conbin = TextDatumGetCString(val); - newcon->qual = (Node *) stringToNode(conbin); - - /* Find or create work queue entry for this table */ - tab = ATGetQueueEntry(wqueue, rel); - tab->constraints = lappend(tab->constraints, newcon); - - /* - * Invalidate relcache so that others see the new validated - * constraint. - */ - CacheInvalidateRelcache(rel); + QueueCheckConstraintValidation(wqueue, conrel, rel, constrName, + tuple, recurse, recursing, lockmode); } - /* - * Now update the catalog, while we have the door open. - */ - copyTuple = heap_copytuple(tuple); - copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple); - copy_con->convalidated = true; - CatalogTupleUpdate(conrel, ©Tuple->t_self, copyTuple); - - InvokeObjectPostAlterHook(ConstraintRelationId, con->oid, 0); - - heap_freetuple(copyTuple); - ObjectAddressSet(address, ConstraintRelationId, con->oid); } else -- 2.43.5 [application/octet-stream] v2-0002-Allow-NOT-VALID-foreign-key-constraints-on-partit.patch (20.0K, ../../CAAJ_b95pjru9JxU4Ve8yRFSCPO8VNmur8jfuHrACWuTBDt97zg@mail.gmail.com/3-v2-0002-Allow-NOT-VALID-foreign-key-constraints-on-partit.patch) download | inline diff: From 1534518b7716487748346ebc58eeae2a2862abb0 Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 2 Jan 2025 19:04:49 +0530 Subject: [PATCH v2 2/2] Allow NOT VALID foreign key constraints on partitioned tables. When merging an existing foreign key during the addition or attachment of a partition, consider the following: 1. If the parent foreign key constraint being added is NOT VALID, the validity flag of the matching child constraint is ignored. It is harmless for the child constraint to remain valid. 2. If the parent foreign key constraint being added is VALID and the matching child constraint is NOT VALID, the we implicitly validates the child constraint, marking it as valid. This behavior is consistent with creating a new constraint on the child table rather than attaching it to an existing one, as it ensures the child data is validated. --- doc/src/sgml/ref/alter_table.sgml | 2 - src/backend/commands/tablecmds.c | 126 +++++++++++++++------- src/test/regress/expected/foreign_key.out | 76 +++++++++++-- src/test/regress/sql/foreign_key.sql | 58 +++++++++- 4 files changed, 214 insertions(+), 48 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 938450fba18..e6cf26d7778 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -486,8 +486,6 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM <para> Additional restrictions apply when unique or primary key constraints are added to partitioned tables; see <link linkend="sql-createtable"><command>CREATE TABLE</command></link>. - Also, foreign key constraints on partitioned - tables may not be declared <literal>NOT VALID</literal> at present. </para> </listitem> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index c0881832eb1..f34d66092dc 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -568,8 +568,9 @@ static void createForeignKeyActionTriggers(Relation rel, Oid refRelOid, Oid indexOid, Oid parentDelTrigger, Oid parentUpdTrigger, Oid *deleteTrigOid, Oid *updateTrigOid); -static bool tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, - Oid partRelid, +static bool tryAttachPartitionForeignKey(List **wqueue, + ForeignKeyCacheInfo *fk, + Relation partition, Oid parentConstrOid, int numfks, AttrNumber *mapped_conkey, AttrNumber *confkey, Oid *conpfeqop, @@ -9766,22 +9767,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * Validity checks (permission checks wait till we have the column * numbers) */ - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - { - if (!recurse) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot use ONLY for foreign key on partitioned table \"%s\" referencing relation \"%s\"", - RelationGetRelationName(rel), - RelationGetRelationName(pkrel)))); - if (fkconstraint->skip_validation && !fkconstraint->initially_valid) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot add NOT VALID foreign key on partitioned table \"%s\" referencing relation \"%s\"", - RelationGetRelationName(rel), - RelationGetRelationName(pkrel)), - errdetail("This feature is not yet supported on partitioned tables."))); - } + if (!recurse && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot use ONLY for foreign key on partitioned table \"%s\" referencing relation \"%s\"", + RelationGetRelationName(rel), + RelationGetRelationName(pkrel)))); if (pkrel->rd_rel->relkind != RELKIND_RELATION && pkrel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) @@ -10774,8 +10765,7 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, */ for (int i = 0; i < pd->nparts; i++) { - Oid partitionId = pd->oids[i]; - Relation partition = table_open(partitionId, lockmode); + Relation partition = table_open(pd->oids[i], lockmode); List *partFKs; AttrMap *attmap; AttrNumber mapped_fkattnum[INDEX_MAX_KEYS]; @@ -10799,8 +10789,9 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, ForeignKeyCacheInfo *fk; fk = lfirst_node(ForeignKeyCacheInfo, cell); - if (tryAttachPartitionForeignKey(fk, - partitionId, + if (tryAttachPartitionForeignKey(wqueue, + fk, + partition, parentConstr, numfks, mapped_fkattnum, @@ -11252,8 +11243,9 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) { ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, lc); - if (tryAttachPartitionForeignKey(fk, - RelationGetRelid(partRel), + if (tryAttachPartitionForeignKey(wqueue, + fk, + partRel, parentConstrOid, numfks, mapped_conkey, @@ -11356,8 +11348,9 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) * return false. */ static bool -tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, - Oid partRelid, +tryAttachPartitionForeignKey(List **wqueue, + ForeignKeyCacheInfo *fk, + Relation partition, Oid parentConstrOid, int numfks, AttrNumber *mapped_conkey, @@ -11376,12 +11369,15 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, HeapTuple trigtup; Oid insertTriggerOid, updateTriggerOid; + bool parentConstrIsValid; + bool partConstrIsValid; parentConstrTup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(parentConstrOid)); if (!HeapTupleIsValid(parentConstrTup)) elog(ERROR, "cache lookup failed for constraint %u", parentConstrOid); parentConstr = (Form_pg_constraint) GETSTRUCT(parentConstrTup); + parentConstrIsValid = parentConstr->convalidated; /* * Do some quick & easy initial checks. If any of these fail, we cannot @@ -11404,17 +11400,18 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, } /* - * Looks good so far; do some more extensive checks. Presumably the check - * for 'convalidated' could be dropped, since we don't really care about - * that, but let's be careful for now. + * Looks good so far; perform more extensive checks except for + * convalidated. There’s no need to worry about it because attaching a + * valid parent constraint to an invalid child constraint will eventually + * trigger implicit data validation for the child. */ partcontup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); if (!HeapTupleIsValid(partcontup)) elog(ERROR, "cache lookup failed for constraint %u", fk->conoid); partConstr = (Form_pg_constraint) GETSTRUCT(partcontup); + partConstrIsValid = parentConstr->convalidated; if (OidIsValid(partConstr->conparentid) || - !partConstr->convalidated || partConstr->condeferrable != parentConstr->condeferrable || partConstr->condeferred != parentConstr->condeferred || partConstr->confupdtype != parentConstr->confupdtype || @@ -11473,7 +11470,8 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, systable_endscan(scan); - ConstraintSetParentConstraint(fk->conoid, parentConstrOid, partRelid); + ConstraintSetParentConstraint(fk->conoid, parentConstrOid, + RelationGetRelid(partition)); /* * Like the constraint, attach partition's "check" triggers to the @@ -11484,10 +11482,10 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, &insertTriggerOid, &updateTriggerOid); Assert(OidIsValid(insertTriggerOid) && OidIsValid(parentInsTrigger)); TriggerSetParentTrigger(trigrel, insertTriggerOid, parentInsTrigger, - partRelid); + RelationGetRelid(partition)); Assert(OidIsValid(updateTriggerOid) && OidIsValid(parentUpdTrigger)); TriggerSetParentTrigger(trigrel, updateTriggerOid, parentUpdTrigger, - partRelid); + RelationGetRelid(partition)); /* * If the referenced table is partitioned, then the partition we're @@ -11565,6 +11563,32 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, } CommandCounterIncrement(); + + /* + * If a valid parent constraint is attached to a NOT VALID child + * constraint, implicitly trigger validation for the child constraint. + * This behavior aligns with creating a new constraint on the child table + * rather than attaching it to the existing one, as it would ultimately + * validate the child data. Conversely, having an invalid parent + * constraint while the child constraint is valid doesn't cause any harm. + */ + if (parentConstrIsValid && !partConstrIsValid); + { + Relation conrel; + + conrel = table_open(ConstraintRelationId, RowExclusiveLock); + + partcontup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); + if (!HeapTupleIsValid(partcontup)) + elog(ERROR, "cache lookup failed for constraint %u", fk->conoid); + + /* Using the same lock as used in AT_ValidateConstraint */ + QueueFKConstraintValidation(wqueue, conrel, partition, partcontup, + ShareUpdateExclusiveLock); + ReleaseSysCache(partcontup); + table_close(conrel, RowExclusiveLock); + } + return true; } @@ -11983,7 +12007,7 @@ ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, * * Add an entry to the wqueue to validate the given foreign key constraint in * Phase 3 and update the convalidated field in the pg_constraint catalog - * for the specified relation. + * for the specified relation and all its children. */ static void QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, @@ -12021,9 +12045,39 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, } /* - * We disallow creating invalid foreign keys to or from partitioned - * tables, so ignoring the recursion bit is okay. + * If the table at either end of the constraint is partitioned, we need to + * recurse and handle every constraint that is a child of this constraint. */ + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE || + get_rel_relkind(con->confrelid) == RELKIND_PARTITIONED_TABLE) + { + ScanKeyData pkey; + SysScanDesc pscan; + HeapTuple childtup; + + ScanKeyInit(&pkey, + Anum_pg_constraint_conparentid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(con->oid)); + + pscan = systable_beginscan(conrel, ConstraintParentIndexId, + true, NULL, 1, &pkey); + + while (HeapTupleIsValid(childtup = systable_getnext(pscan))) + { + Form_pg_constraint childcon; + Relation childrel; + + childcon = (Form_pg_constraint) GETSTRUCT(childtup); + childrel = table_open(childcon->conrelid, lockmode); + + QueueFKConstraintValidation(wqueue, conrel, childrel, childtup, + lockmode); + table_close(childrel, NoLock); + } + + systable_endscan(pscan); + } /* * Now update the catalog, while we have the door open. diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 3f459f70ac1..f617d519aef 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1571,6 +1571,76 @@ drop table pktable2, fktable2; -- -- Foreign keys and partitioned tables -- +-- NOT VALID foreign key on partitioned table +CREATE TABLE fk_notpartitioned_pk (a int, b int, PRIMARY KEY (a, b)); +CREATE TABLE fk_partitioned_fk (b int, a int) PARTITION BY RANGE (a, b); +ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; +-- Attaching a child table with the same valid foreign key constraint. +CREATE TABLE fk_partitioned_fk_1 (a int, b int); +ALTER TABLE fk_partitioned_fk_1 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_1 FOR VALUES FROM (0,0) TO (1000,1000); +-- Child constraint will remain valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + conname | convalidated | conrelid +------------------------------+--------------+--------------------- + fk_partitioned_fk_a_b_fkey | f | fk_partitioned_fk + fk_partitioned_fk_1_a_b_fkey | t | fk_partitioned_fk_1 +(2 rows) + +-- Validate the constraint +ALTER TABLE fk_partitioned_fk VALIDATE CONSTRAINT fk_partitioned_fk_a_b_fkey; +-- All constraints are now valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + conname | convalidated | conrelid +------------------------------+--------------+--------------------- + fk_partitioned_fk_a_b_fkey | t | fk_partitioned_fk + fk_partitioned_fk_1_a_b_fkey | t | fk_partitioned_fk_1 +(2 rows) + +-- Attaching a child with a NOT VALID constraint. +CREATE TABLE fk_partitioned_fk_2 (a int, b int); +INSERT INTO fk_partitioned_fk_2 VALUES(1000, 1000); -- doesn't exist in referenced table +ALTER TABLE fk_partitioned_fk_2 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; +-- It will fail because the attach operation implicitly validates the data. +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000); +ERROR: insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_b_fkey" +DETAIL: Key (a, b)=(1000, 1000) is not present in table "fk_notpartitioned_pk". +-- Remove the invalid data and try again. +TRUNCATE fk_partitioned_fk_2; +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000); +-- The child constraint will also be valid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_partitioned_fk_2'::regclass; + conname | convalidated +------------------------------+-------------- + fk_partitioned_fk_2_a_b_fkey | t +(1 row) + +DROP TABLE fk_partitioned_fk, fk_notpartitioned_pk; +-- NOT VALID foreign key on a non-partitioned table referencing a partitioned table +CREATE TABLE fk_partitioned_pk (a int, b int, PRIMARY KEY (a, b)) PARTITION BY RANGE (a, b); +CREATE TABLE fk_partitioned_pk_1 PARTITION OF fk_partitioned_pk FOR VALUES FROM (0,0) TO (1000,1000); +CREATE TABLE fk_notpartitioned_fk (b int, a int); +ALTER TABLE fk_notpartitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_partitioned_pk NOT VALID; +-- Constraint will be invalid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_notpartitioned_fk'::regclass; + conname | convalidated +--------------------------------+-------------- + fk_notpartitioned_fk_a_b_fkey | f + fk_notpartitioned_fk_a_b_fkey1 | f +(2 rows) + +ALTER TABLE fk_notpartitioned_fk VALIDATE CONSTRAINT fk_notpartitioned_fk_a_b_fkey; +-- All constraints are now valid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_notpartitioned_fk'::regclass; + conname | convalidated +--------------------------------+-------------- + fk_notpartitioned_fk_a_b_fkey | t + fk_notpartitioned_fk_a_b_fkey1 | t +(2 rows) + +DROP TABLE fk_notpartitioned_fk, fk_partitioned_pk; -- Creation of a partitioned hierarchy with irregular definitions CREATE TABLE fk_notpartitioned_pk (fdrop1 int, a int, fdrop2 int, b int, PRIMARY KEY (a, b)); @@ -1597,12 +1667,6 @@ ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3 ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; ERROR: cannot use ONLY for foreign key on partitioned table "fk_partitioned_fk" referencing relation "fk_notpartitioned_pk" --- Adding a NOT VALID foreign key on a partitioned table referencing --- a non-partitioned table fails. -ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) - REFERENCES fk_notpartitioned_pk NOT VALID; -ERROR: cannot add NOT VALID foreign key on partitioned table "fk_partitioned_fk" referencing relation "fk_notpartitioned_pk" -DETAIL: This feature is not yet supported on partitioned tables. -- these inserts, targeting both the partition directly as well as the -- partitioned table, should all fail INSERT INTO fk_partitioned_fk (a,b) VALUES (500, 501); diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 2e710e419c2..d5f68812ba6 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1173,6 +1173,60 @@ drop table pktable2, fktable2; -- Foreign keys and partitioned tables -- +-- NOT VALID foreign key on partitioned table +CREATE TABLE fk_notpartitioned_pk (a int, b int, PRIMARY KEY (a, b)); +CREATE TABLE fk_partitioned_fk (b int, a int) PARTITION BY RANGE (a, b); +ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; + +-- Attaching a child table with the same valid foreign key constraint. +CREATE TABLE fk_partitioned_fk_1 (a int, b int); +ALTER TABLE fk_partitioned_fk_1 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_1 FOR VALUES FROM (0,0) TO (1000,1000); + +-- Child constraint will remain valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + +-- Validate the constraint +ALTER TABLE fk_partitioned_fk VALIDATE CONSTRAINT fk_partitioned_fk_a_b_fkey; + +-- All constraints are now valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + +-- Attaching a child with a NOT VALID constraint. +CREATE TABLE fk_partitioned_fk_2 (a int, b int); +INSERT INTO fk_partitioned_fk_2 VALUES(1000, 1000); -- doesn't exist in referenced table +ALTER TABLE fk_partitioned_fk_2 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; + +-- It will fail because the attach operation implicitly validates the data. +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000); + +-- Remove the invalid data and try again. +TRUNCATE fk_partitioned_fk_2; +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000); + +-- The child constraint will also be valid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_partitioned_fk_2'::regclass; + +DROP TABLE fk_partitioned_fk, fk_notpartitioned_pk; + +-- NOT VALID foreign key on a non-partitioned table referencing a partitioned table +CREATE TABLE fk_partitioned_pk (a int, b int, PRIMARY KEY (a, b)) PARTITION BY RANGE (a, b); +CREATE TABLE fk_partitioned_pk_1 PARTITION OF fk_partitioned_pk FOR VALUES FROM (0,0) TO (1000,1000); +CREATE TABLE fk_notpartitioned_fk (b int, a int); +ALTER TABLE fk_notpartitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_partitioned_pk NOT VALID; + +-- Constraint will be invalid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_notpartitioned_fk'::regclass; + +ALTER TABLE fk_notpartitioned_fk VALIDATE CONSTRAINT fk_notpartitioned_fk_a_b_fkey; + +-- All constraints are now valid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_notpartitioned_fk'::regclass; + +DROP TABLE fk_notpartitioned_fk, fk_partitioned_pk; + -- Creation of a partitioned hierarchy with irregular definitions CREATE TABLE fk_notpartitioned_pk (fdrop1 int, a int, fdrop2 int, b int, PRIMARY KEY (a, b)); @@ -1200,10 +1254,6 @@ ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3 -- a non-partitioned table fails. ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; --- Adding a NOT VALID foreign key on a partitioned table referencing --- a non-partitioned table fails. -ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) - REFERENCES fk_notpartitioned_pk NOT VALID; -- these inserts, targeting both the partition directly as well as the -- partitioned table, should all fail -- 2.43.5 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Allow NOT VALID foreign key constraints on partitioned tables. @ 2025-01-21 21:06 Álvaro Herrera <[email protected]> parent: Amul Sul <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Álvaro Herrera @ 2025-01-21 21:06 UTC (permalink / raw) To: Amul Sul <[email protected]>; +Cc: [email protected] <[email protected]> Suppose I have a hierarchy like this parent | child /\ / \ / grandchild2 / grandchild1 and I have a validated constraint on grandchild1 and an invalid constraint on child. What happens if I add a constraint on parent? In my understanding, it should not attempt to revalidate the constraint on grandchild1, because it's known valid; but I don't think I see code that would skip validation there. That is, QueueFKConstraintValidation does its thing unconditionally (esp. recursing to children), which seems wrong. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "I am amazed at [the pgsql-sql] mailing list for the wonderful support, and lack of hesitasion in answering a lost soul's question, I just wished the rest of the mailing list could be like this." (Fotis) https://postgr.es/m/[email protected] ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Allow NOT VALID foreign key constraints on partitioned tables. @ 2025-01-22 06:12 Amul Sul <[email protected]> parent: Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Amul Sul @ 2025-01-22 06:12 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: [email protected] <[email protected]> On Wed, Jan 22, 2025 at 2:36 AM Álvaro Herrera <[email protected]> wrote: > > > Suppose I have a hierarchy like this > > parent > | > child > /\ > / \ > / grandchild2 > / > grandchild1 > > and I have a validated constraint on grandchild1 and an invalid > constraint on child. What happens if I add a constraint on parent? In > my understanding, it should not attempt to revalidate the constraint on > grandchild1, because it's known valid; but I don't think I see code that > would skip validation there. That is, QueueFKConstraintValidation does > its thing unconditionally (esp. recursing to children), which seems > wrong. > You’re correct; it’s fixed in the attached version, along with an assert(!convalidated) in QueueFKConstraintValidation(), and I’ve included tests to cover the change. Thanks for reviewing this patch. Regards, Amul Attachments: [application/octet-stream] v3-0002-Allow-NOT-VALID-foreign-key-constraints-on-partit.patch (22.8K, ../../CAAJ_b97_zJvzmLJS8AAzGFEYAAMbm5KM8ix4VPxaSO=ptkYx6Q@mail.gmail.com/2-v3-0002-Allow-NOT-VALID-foreign-key-constraints-on-partit.patch) download | inline diff: From 41af6dc0782b9c1516d4e4a81f809c3eb432d08b Mon Sep 17 00:00:00 2001 From: Amul Sul <[email protected]> Date: Thu, 2 Jan 2025 19:04:49 +0530 Subject: [PATCH v3] Allow NOT VALID foreign key constraints on partitioned tables. When merging an existing foreign key during the addition or attachment of a partition, consider the following: 1. If the parent foreign key constraint being added is NOT VALID, the validity flag of the matching child constraint is ignored. It is harmless for the child constraint to remain valid. 2. If the parent foreign key constraint being added is VALID and the matching child constraint is NOT VALID, the we implicitly validates the child constraint, marking it as valid. This behavior is consistent with creating a new constraint on the child table rather than attaching it to an existing one, as it ensures the child data is validated. --- doc/src/sgml/ref/alter_table.sgml | 2 - src/backend/commands/tablecmds.c | 136 ++++++++++++++++------ src/test/regress/expected/foreign_key.out | 97 ++++++++++++++- src/test/regress/sql/foreign_key.sql | 72 +++++++++++- 4 files changed, 259 insertions(+), 48 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index df4f5d5bbd8..f9576da435e 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -486,8 +486,6 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM <para> Additional restrictions apply when unique or primary key constraints are added to partitioned tables; see <link linkend="sql-createtable"><command>CREATE TABLE</command></link>. - Also, foreign key constraints on partitioned - tables may not be declared <literal>NOT VALID</literal> at present. </para> </listitem> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d8f0a99ad93..6fac2c739e8 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -574,8 +574,9 @@ static void createForeignKeyActionTriggers(Relation rel, Oid refRelOid, Oid indexOid, Oid parentDelTrigger, Oid parentUpdTrigger, Oid *deleteTrigOid, Oid *updateTrigOid); -static bool tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, - Oid partRelid, +static bool tryAttachPartitionForeignKey(List **wqueue, + ForeignKeyCacheInfo *fk, + Relation partition, Oid parentConstrOid, int numfks, AttrNumber *mapped_conkey, AttrNumber *confkey, Oid *conpfeqop, @@ -9772,22 +9773,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * Validity checks (permission checks wait till we have the column * numbers) */ - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - { - if (!recurse) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot use ONLY for foreign key on partitioned table \"%s\" referencing relation \"%s\"", - RelationGetRelationName(rel), - RelationGetRelationName(pkrel)))); - if (fkconstraint->skip_validation && !fkconstraint->initially_valid) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot add NOT VALID foreign key on partitioned table \"%s\" referencing relation \"%s\"", - RelationGetRelationName(rel), - RelationGetRelationName(pkrel)), - errdetail("This feature is not yet supported on partitioned tables."))); - } + if (!recurse && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot use ONLY for foreign key on partitioned table \"%s\" referencing relation \"%s\"", + RelationGetRelationName(rel), + RelationGetRelationName(pkrel)))); if (pkrel->rd_rel->relkind != RELKIND_RELATION && pkrel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) @@ -10782,8 +10773,7 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, */ for (int i = 0; i < pd->nparts; i++) { - Oid partitionId = pd->oids[i]; - Relation partition = table_open(partitionId, lockmode); + Relation partition = table_open(pd->oids[i], lockmode); List *partFKs; AttrMap *attmap; AttrNumber mapped_fkattnum[INDEX_MAX_KEYS]; @@ -10807,8 +10797,9 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, ForeignKeyCacheInfo *fk; fk = lfirst_node(ForeignKeyCacheInfo, cell); - if (tryAttachPartitionForeignKey(fk, - partitionId, + if (tryAttachPartitionForeignKey(wqueue, + fk, + partition, parentConstr, numfks, mapped_fkattnum, @@ -11260,8 +11251,9 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) { ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, lc); - if (tryAttachPartitionForeignKey(fk, - RelationGetRelid(partRel), + if (tryAttachPartitionForeignKey(wqueue, + fk, + partRel, parentConstrOid, numfks, mapped_conkey, @@ -11364,8 +11356,9 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) * return false. */ static bool -tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, - Oid partRelid, +tryAttachPartitionForeignKey(List **wqueue, + ForeignKeyCacheInfo *fk, + Relation partition, Oid parentConstrOid, int numfks, AttrNumber *mapped_conkey, @@ -11384,12 +11377,15 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, HeapTuple trigtup; Oid insertTriggerOid, updateTriggerOid; + bool parentConstrIsValid; + bool partConstrIsValid; parentConstrTup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(parentConstrOid)); if (!HeapTupleIsValid(parentConstrTup)) elog(ERROR, "cache lookup failed for constraint %u", parentConstrOid); parentConstr = (Form_pg_constraint) GETSTRUCT(parentConstrTup); + parentConstrIsValid = parentConstr->convalidated; /* * Do some quick & easy initial checks. If any of these fail, we cannot @@ -11412,17 +11408,18 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, } /* - * Looks good so far; do some more extensive checks. Presumably the check - * for 'convalidated' could be dropped, since we don't really care about - * that, but let's be careful for now. + * Looks good so far; perform more extensive checks except for + * convalidated. There's no need to worry about it because attaching a + * valid parent constraint to an invalid child constraint will eventually + * trigger implicit data validation for the child. */ partcontup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); if (!HeapTupleIsValid(partcontup)) elog(ERROR, "cache lookup failed for constraint %u", fk->conoid); partConstr = (Form_pg_constraint) GETSTRUCT(partcontup); + partConstrIsValid = partConstr->convalidated; if (OidIsValid(partConstr->conparentid) || - !partConstr->convalidated || partConstr->condeferrable != parentConstr->condeferrable || partConstr->condeferred != parentConstr->condeferred || partConstr->confupdtype != parentConstr->confupdtype || @@ -11481,7 +11478,8 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, systable_endscan(scan); - ConstraintSetParentConstraint(fk->conoid, parentConstrOid, partRelid); + ConstraintSetParentConstraint(fk->conoid, parentConstrOid, + RelationGetRelid(partition)); /* * Like the constraint, attach partition's "check" triggers to the @@ -11492,10 +11490,10 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, &insertTriggerOid, &updateTriggerOid); Assert(OidIsValid(insertTriggerOid) && OidIsValid(parentInsTrigger)); TriggerSetParentTrigger(trigrel, insertTriggerOid, parentInsTrigger, - partRelid); + RelationGetRelid(partition)); Assert(OidIsValid(updateTriggerOid) && OidIsValid(parentUpdTrigger)); TriggerSetParentTrigger(trigrel, updateTriggerOid, parentUpdTrigger, - partRelid); + RelationGetRelid(partition)); /* * If the referenced table is partitioned, then the partition we're @@ -11573,6 +11571,32 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, } CommandCounterIncrement(); + + /* + * If a valid parent constraint is attached to a NOT VALID child + * constraint, implicitly trigger validation for the child constraint. + * This behavior aligns with creating a new constraint on the child table + * rather than attaching it to the existing one, as it would ultimately + * validate the child data. Conversely, having an invalid parent + * constraint while the child constraint is valid doesn't cause any harm. + */ + if (parentConstrIsValid && !partConstrIsValid) + { + Relation conrel; + + conrel = table_open(ConstraintRelationId, RowExclusiveLock); + + partcontup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); + if (!HeapTupleIsValid(partcontup)) + elog(ERROR, "cache lookup failed for constraint %u", fk->conoid); + + /* Using the same lock as used in AT_ValidateConstraint */ + QueueFKConstraintValidation(wqueue, conrel, partition, partcontup, + ShareUpdateExclusiveLock); + ReleaseSysCache(partcontup); + table_close(conrel, RowExclusiveLock); + } + return true; } @@ -12113,7 +12137,7 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, * * Add an entry to the wqueue to validate the given foreign key constraint in * Phase 3 and update the convalidated field in the pg_constraint catalog - * for the specified relation. + * for the specified relation and all its children. */ static void QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, @@ -12126,6 +12150,7 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, con = (Form_pg_constraint) GETSTRUCT(contuple); Assert(con->contype == CONSTRAINT_FOREIGN); + Assert(!con->convalidated); if (rel->rd_rel->relkind == RELKIND_RELATION) { @@ -12151,9 +12176,48 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, } /* - * We disallow creating invalid foreign keys to or from partitioned - * tables, so ignoring the recursion bit is okay. + * If the table at either end of the constraint is partitioned, we need to + * recurse and handle every constraint that is a child of this constraint. */ + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE || + get_rel_relkind(con->confrelid) == RELKIND_PARTITIONED_TABLE) + { + ScanKeyData pkey; + SysScanDesc pscan; + HeapTuple childtup; + + ScanKeyInit(&pkey, + Anum_pg_constraint_conparentid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(con->oid)); + + pscan = systable_beginscan(conrel, ConstraintParentIndexId, + true, NULL, 1, &pkey); + + while (HeapTupleIsValid(childtup = systable_getnext(pscan))) + { + Form_pg_constraint childcon; + Relation childrel; + + childcon = (Form_pg_constraint) GETSTRUCT(childtup); + + /* + * If the child constraint has already been validated, no further + * action is required for it or its descendants, as they are all + * valid. + */ + if (childcon->convalidated) + continue; + + childrel = table_open(childcon->conrelid, lockmode); + + QueueFKConstraintValidation(wqueue, conrel, childrel, childtup, + lockmode); + table_close(childrel, NoLock); + } + + systable_endscan(pscan); + } /* * Now update the catalog, while we have the door open. diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index ee4cf85fda9..da339ed5a3c 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1571,6 +1571,97 @@ drop table pktable2, fktable2; -- -- Foreign keys and partitioned tables -- +-- NOT VALID foreign key on partitioned table +CREATE TABLE fk_notpartitioned_pk (a int, b int, PRIMARY KEY (a, b)); +CREATE TABLE fk_partitioned_fk (b int, a int) PARTITION BY RANGE (a, b); +ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; +-- Attaching a child table with the same valid foreign key constraint. +CREATE TABLE fk_partitioned_fk_1 (a int, b int); +ALTER TABLE fk_partitioned_fk_1 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_1 FOR VALUES FROM (0,0) TO (1000,1000); +-- Child constraint will remain valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + conname | convalidated | conrelid +------------------------------+--------------+--------------------- + fk_partitioned_fk_a_b_fkey | f | fk_partitioned_fk + fk_partitioned_fk_1_a_b_fkey | t | fk_partitioned_fk_1 +(2 rows) + +-- Validate the constraint +ALTER TABLE fk_partitioned_fk VALIDATE CONSTRAINT fk_partitioned_fk_a_b_fkey; +-- All constraints are now valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + conname | convalidated | conrelid +------------------------------+--------------+--------------------- + fk_partitioned_fk_a_b_fkey | t | fk_partitioned_fk + fk_partitioned_fk_1_a_b_fkey | t | fk_partitioned_fk_1 +(2 rows) + +-- Attaching a child with a NOT VALID constraint. +CREATE TABLE fk_partitioned_fk_2 (a int, b int); +INSERT INTO fk_partitioned_fk_2 VALUES(1000, 1000); -- doesn't exist in referenced table +ALTER TABLE fk_partitioned_fk_2 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; +-- It will fail because the attach operation implicitly validates the data. +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000); +ERROR: insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_b_fkey" +DETAIL: Key (a, b)=(1000, 1000) is not present in table "fk_notpartitioned_pk". +-- Remove the invalid data and try again. +TRUNCATE fk_partitioned_fk_2; +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000); +-- The child constraint will also be valid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_partitioned_fk_2'::regclass; + conname | convalidated +------------------------------+-------------- + fk_partitioned_fk_2_a_b_fkey | t +(1 row) + +-- Test case where the child constraint is invalid, the grandchild constraint +-- is valid, and the validation for the grandchild should be skipped when a +-- valid constraint is applied to the top parent. +CREATE TABLE fk_partitioned_fk_3 (a int, b int) PARTITION BY RANGE (a, b); +ALTER TABLE fk_partitioned_fk_3 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; +CREATE TABLE fk_partitioned_fk_3_1 (a int, b int); +ALTER TABLE fk_partitioned_fk_3_1 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; +ALTER TABLE fk_partitioned_fk_3 ATTACH PARTITION fk_partitioned_fk_3_1 FOR VALUES FROM (2000,2000) TO (3000,3000); +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3 FOR VALUES FROM (2000,2000) TO (3000,3000); +-- All constraints are now valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + conname | convalidated | conrelid +--------------------------------+--------------+----------------------- + fk_partitioned_fk_a_b_fkey | t | fk_partitioned_fk + fk_partitioned_fk_1_a_b_fkey | t | fk_partitioned_fk_1 + fk_partitioned_fk_2_a_b_fkey | t | fk_partitioned_fk_2 + fk_partitioned_fk_3_a_b_fkey | t | fk_partitioned_fk_3 + fk_partitioned_fk_3_1_a_b_fkey | t | fk_partitioned_fk_3_1 +(5 rows) + +DROP TABLE fk_partitioned_fk, fk_notpartitioned_pk; +-- NOT VALID foreign key on a non-partitioned table referencing a partitioned table +CREATE TABLE fk_partitioned_pk (a int, b int, PRIMARY KEY (a, b)) PARTITION BY RANGE (a, b); +CREATE TABLE fk_partitioned_pk_1 PARTITION OF fk_partitioned_pk FOR VALUES FROM (0,0) TO (1000,1000); +CREATE TABLE fk_notpartitioned_fk (b int, a int); +ALTER TABLE fk_notpartitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_partitioned_pk NOT VALID; +-- Constraint will be invalid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_notpartitioned_fk'::regclass; + conname | convalidated +--------------------------------+-------------- + fk_notpartitioned_fk_a_b_fkey | f + fk_notpartitioned_fk_a_b_fkey1 | f +(2 rows) + +ALTER TABLE fk_notpartitioned_fk VALIDATE CONSTRAINT fk_notpartitioned_fk_a_b_fkey; +-- All constraints are now valid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_notpartitioned_fk'::regclass; + conname | convalidated +--------------------------------+-------------- + fk_notpartitioned_fk_a_b_fkey | t + fk_notpartitioned_fk_a_b_fkey1 | t +(2 rows) + +DROP TABLE fk_notpartitioned_fk, fk_partitioned_pk; -- Creation of a partitioned hierarchy with irregular definitions CREATE TABLE fk_notpartitioned_pk (fdrop1 int, a int, fdrop2 int, b int, PRIMARY KEY (a, b)); @@ -1597,12 +1688,6 @@ ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3 ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; ERROR: cannot use ONLY for foreign key on partitioned table "fk_partitioned_fk" referencing relation "fk_notpartitioned_pk" --- Adding a NOT VALID foreign key on a partitioned table referencing --- a non-partitioned table fails. -ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) - REFERENCES fk_notpartitioned_pk NOT VALID; -ERROR: cannot add NOT VALID foreign key on partitioned table "fk_partitioned_fk" referencing relation "fk_notpartitioned_pk" -DETAIL: This feature is not yet supported on partitioned tables. -- these inserts, targeting both the partition directly as well as the -- partitioned table, should all fail INSERT INTO fk_partitioned_fk (a,b) VALUES (500, 501); diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 8c4e4c7c833..46fdee8b20d 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1173,6 +1173,74 @@ drop table pktable2, fktable2; -- Foreign keys and partitioned tables -- +-- NOT VALID foreign key on partitioned table +CREATE TABLE fk_notpartitioned_pk (a int, b int, PRIMARY KEY (a, b)); +CREATE TABLE fk_partitioned_fk (b int, a int) PARTITION BY RANGE (a, b); +ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; + +-- Attaching a child table with the same valid foreign key constraint. +CREATE TABLE fk_partitioned_fk_1 (a int, b int); +ALTER TABLE fk_partitioned_fk_1 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_1 FOR VALUES FROM (0,0) TO (1000,1000); + +-- Child constraint will remain valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + +-- Validate the constraint +ALTER TABLE fk_partitioned_fk VALIDATE CONSTRAINT fk_partitioned_fk_a_b_fkey; + +-- All constraints are now valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + +-- Attaching a child with a NOT VALID constraint. +CREATE TABLE fk_partitioned_fk_2 (a int, b int); +INSERT INTO fk_partitioned_fk_2 VALUES(1000, 1000); -- doesn't exist in referenced table +ALTER TABLE fk_partitioned_fk_2 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; + +-- It will fail because the attach operation implicitly validates the data. +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000); + +-- Remove the invalid data and try again. +TRUNCATE fk_partitioned_fk_2; +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES FROM (1000,1000) TO (2000,2000); + +-- The child constraint will also be valid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_partitioned_fk_2'::regclass; + +-- Test case where the child constraint is invalid, the grandchild constraint +-- is valid, and the validation for the grandchild should be skipped when a +-- valid constraint is applied to the top parent. +CREATE TABLE fk_partitioned_fk_3 (a int, b int) PARTITION BY RANGE (a, b); +ALTER TABLE fk_partitioned_fk_3 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; +CREATE TABLE fk_partitioned_fk_3_1 (a int, b int); +ALTER TABLE fk_partitioned_fk_3_1 ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; +ALTER TABLE fk_partitioned_fk_3 ATTACH PARTITION fk_partitioned_fk_3_1 FOR VALUES FROM (2000,2000) TO (3000,3000); +ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3 FOR VALUES FROM (2000,2000) TO (3000,3000); + +-- All constraints are now valid. +SELECT conname, convalidated, conrelid::regclass FROM pg_constraint +WHERE conrelid::regclass::text like 'fk_partitioned_fk%' ORDER BY oid; + +DROP TABLE fk_partitioned_fk, fk_notpartitioned_pk; + +-- NOT VALID foreign key on a non-partitioned table referencing a partitioned table +CREATE TABLE fk_partitioned_pk (a int, b int, PRIMARY KEY (a, b)) PARTITION BY RANGE (a, b); +CREATE TABLE fk_partitioned_pk_1 PARTITION OF fk_partitioned_pk FOR VALUES FROM (0,0) TO (1000,1000); +CREATE TABLE fk_notpartitioned_fk (b int, a int); +ALTER TABLE fk_notpartitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_partitioned_pk NOT VALID; + +-- Constraint will be invalid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_notpartitioned_fk'::regclass; + +ALTER TABLE fk_notpartitioned_fk VALIDATE CONSTRAINT fk_notpartitioned_fk_a_b_fkey; + +-- All constraints are now valid. +SELECT conname, convalidated FROM pg_constraint WHERE conrelid = 'fk_notpartitioned_fk'::regclass; + +DROP TABLE fk_notpartitioned_fk, fk_partitioned_pk; + -- Creation of a partitioned hierarchy with irregular definitions CREATE TABLE fk_notpartitioned_pk (fdrop1 int, a int, fdrop2 int, b int, PRIMARY KEY (a, b)); @@ -1200,10 +1268,6 @@ ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3 -- a non-partitioned table fails. ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk; --- Adding a NOT VALID foreign key on a partitioned table referencing --- a non-partitioned table fails. -ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) - REFERENCES fk_notpartitioned_pk NOT VALID; -- these inserts, targeting both the partition directly as well as the -- partitioned table, should all fail -- 2.43.5 ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2025-01-22 06:12 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2018-11-28 14:52 [PATCH v6 1/3] Rework deleteObjectsInList to allow objtype-specific checks Alvaro Herrera <[email protected]> 2024-08-26 04:32 [PATCH v21 2/8] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]> 2025-01-06 04:23 Re: Allow NOT VALID foreign key constraints on partitioned tables. Amul Sul <[email protected]> 2025-01-15 05:34 ` Re: Allow NOT VALID foreign key constraints on partitioned tables. Amul Sul <[email protected]> 2025-01-21 21:06 ` Re: Allow NOT VALID foreign key constraints on partitioned tables. Álvaro Herrera <[email protected]> 2025-01-22 06:12 ` Re: Allow NOT VALID foreign key constraints on partitioned tables. Amul Sul <[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