Constraint merge and not valid status
Hi,
Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion. If merged, the constraint is not checked
for the child data even though it may have some. Is that an oversight?
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 7/13/16 4:22 AM, Amit Langote wrote:
Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion. If merged, the constraint is not checked
for the child data even though it may have some. Is that an oversight?
Seen as how you used to be able to illegally twerk NOT NULL status on
children (and maybe still can), I'd bet this is a bug...
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion. If merged, the constraint is not checked
for the child data even though it may have some. Is that an oversight?
Seems like it. I'd recommend we just error out in that case and tell
the user that they should validate the child's constraint first.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016/07/22 0:38, Robert Haas wrote:
On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion. If merged, the constraint is not checked
for the child data even though it may have some. Is that an oversight?Seems like it. I'd recommend we just error out in that case and tell
the user that they should validate the child's constraint first.
Agreed.
Patch attached. In addition to the recursion from parent case, this seems
to be broken for the alter table child inherit parent case as well. So,
fixed both MergeWithExistingConstraint (called from
AddRelationNewConstraints) and MergeConstraintsIntoExisting (called from
ATExecAddInherit). I had to add a new argument is_not_valid to the former
to signal whether the constraint being propagated itself is declared NOT
VALID, in which we can proceed with merging. Also added some tests for
both cases.
Thanks,
Amit
Attachments:
error-on-merge-with-not-valid-con-1.patchtext/x-diff; name=error-on-merge-with-not-valid-con-1.patchDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index e997b57..f6f0691 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -105,7 +105,7 @@ static void StoreConstraints(Relation rel, List *cooked_constraints,
bool is_internal);
static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
bool allow_merge, bool is_local,
- bool is_no_inherit);
+ bool is_no_inherit, bool is_not_valid);
static void SetRelationNumChecks(Relation rel, int numchecks);
static Node *cookConstraint(ParseState *pstate,
Node *raw_constraint,
@@ -2301,7 +2301,8 @@ AddRelationNewConstraints(Relation rel,
*/
if (MergeWithExistingConstraint(rel, ccname, expr,
allow_merge, is_local,
- cdef->is_no_inherit))
+ cdef->is_no_inherit,
+ cdef->skip_validation))
continue;
}
else
@@ -2389,7 +2390,7 @@ AddRelationNewConstraints(Relation rel,
static bool
MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
bool allow_merge, bool is_local,
- bool is_no_inherit)
+ bool is_no_inherit, bool is_not_valid)
{
bool found;
Relation conDesc;
@@ -2452,6 +2453,14 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"",
ccname, RelationGetRelationName(rel))));
+ /* Cannot merge if the child's constraint is yet invalid. */
+ if (!is_local && !is_not_valid && !con->convalidated)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot merge constraint \"%s\" with NOT VALID constraint on relation \"%s\"",
+ ccname, RelationGetRelationName(rel)),
+ errhint("Please validate the constraint on the child table first.")));
+
if (is_local)
con->conislocal = true;
else
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86e9814..5e72029 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10380,6 +10380,14 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
NameStr(child_con->conname),
RelationGetRelationName(child_rel))));
+ /* Cannot merge if the child's constraint is yet invalid. */
+ if (parent_con->convalidated && !child_con->convalidated)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot merge constraint \"%s\" with NOT VALID constraint on child table \"%s\"",
+ NameStr(parent_con->conname), RelationGetRelationName(child_rel)),
+ errhint("Please validate the constraint on the child table first.")));
+
/*
* OK, bump the child constraint's inheritance count. (If we fail
* later on, this change will just roll back.)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 3232cda..9f13c47 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -460,6 +460,43 @@ Check constraints:
Inherits: nv_parent
-- we leave nv_parent and children around to help test pg_dump logic
+-- prevent merge of constraint with NOT VALID constraint on a child table
+create table parent(a int);
+create table child () inherits (parent);
+alter table child add constraint chk_a check (a > 0) not valid;
+-- ok to merge a NOT VALID constraint itself
+alter table parent add constraint chk_a check (a > 0) not valid;
+NOTICE: merging constraint "chk_a" with inherited definition
+alter table parent drop constraint chk_a;
+-- error
+alter table parent add constraint chk_a check (a > 0);
+ERROR: cannot merge constraint "chk_a" with NOT VALID constraint on relation "child"
+HINT: Please validate the constraint on the child table first.
+-- ok to merge upon validate
+alter table child validate constraint chk_a;
+alter table parent add constraint chk_a check (a > 0);
+NOTICE: merging constraint "chk_a" with inherited definition
+-- check the ALTER TABLE INHERIT case as well
+drop table child;
+create table child(a int);
+alter table child add constraint chk_a check (a > 0) not valid;
+-- ok to merge a NOT VALID constraint itself
+alter table parent drop constraint chk_a;
+alter table parent add constraint chk_a check (a > 0) not valid;
+alter table child inherit parent;
+alter table child no inherit parent;
+alter table parent drop constraint chk_a;
+alter table parent add constraint chk_a check (a > 0);
+-- error
+alter table child inherit parent;
+ERROR: cannot merge constraint "chk_a" with NOT VALID constraint on child table "child"
+HINT: Please validate the constraint on the child table first.
+-- ok to merge upon validate
+alter table child validate constraint chk_a;
+alter table child inherit parent;
+-- clean up
+drop table parent cascade;
+NOTICE: drop cascades to table child
-- Foreign key adding test with mixed types
-- Note: these tables are TEMP to avoid name conflicts when this test
-- is run in parallel with foreign_key.sql.
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 72e65d4..55bf2e9 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -348,6 +348,45 @@ alter table nv_parent add check (d between '2001-01-01'::date and '2099-12-31'::
\d nv_child_2009
-- we leave nv_parent and children around to help test pg_dump logic
+-- prevent merge of constraint with NOT VALID constraint on a child table
+
+create table parent(a int);
+create table child () inherits (parent);
+alter table child add constraint chk_a check (a > 0) not valid;
+
+-- ok to merge a NOT VALID constraint itself
+alter table parent add constraint chk_a check (a > 0) not valid;
+
+alter table parent drop constraint chk_a;
+-- error
+alter table parent add constraint chk_a check (a > 0);
+-- ok to merge upon validate
+alter table child validate constraint chk_a;
+alter table parent add constraint chk_a check (a > 0);
+
+-- check the ALTER TABLE INHERIT case as well
+
+drop table child;
+create table child(a int);
+alter table child add constraint chk_a check (a > 0) not valid;
+
+-- ok to merge a NOT VALID constraint itself
+alter table parent drop constraint chk_a;
+alter table parent add constraint chk_a check (a > 0) not valid;
+alter table child inherit parent;
+
+alter table child no inherit parent;
+alter table parent drop constraint chk_a;
+alter table parent add constraint chk_a check (a > 0);
+-- error
+alter table child inherit parent;
+-- ok to merge upon validate
+alter table child validate constraint chk_a;
+alter table child inherit parent;
+
+-- clean up
+drop table parent cascade;
+
-- Foreign key adding test with mixed types
-- Note: these tables are TEMP to avoid name conflicts when this test
Hello,
At Fri, 22 Jul 2016 14:10:48 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <b5e5d627-2fee-46f5-c219-9915416a9196@lab.ntt.co.jp>
On 2016/07/22 0:38, Robert Haas wrote:
On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion. If merged, the constraint is not checked
for the child data even though it may have some. Is that an oversight?Seems like it. I'd recommend we just error out in that case and tell
the user that they should validate the child's constraint first.Agreed.
Patch attached. In addition to the recursion from parent case, this seems
to be broken for the alter table child inherit parent case as well. So,
fixed both MergeWithExistingConstraint (called from
AddRelationNewConstraints) and MergeConstraintsIntoExisting (called from
ATExecAddInherit). I had to add a new argument is_not_valid to the former
to signal whether the constraint being propagated itself is declared NOT
VALID, in which we can proceed with merging. Also added some tests for
both cases.
It seems to work as expected and message seems to be
reasonable. Test seems to be fine.
By the way I have one question.
Is it an expected configuration where tables in an inheritance
tree has different valid state on the same (check) constraint?
The check should be an equality if it's not.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello,
On 2016/07/22 17:06, Kyotaro HORIGUCHI wrote:
At Fri, 22 Jul 2016 14:10:48 +0900, Amit Langote wrote:
On 2016/07/22 0:38, Robert Haas wrote:
On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote wrote:
Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion. If merged, the constraint is not checked
for the child data even though it may have some. Is that an oversight?Seems like it. I'd recommend we just error out in that case and tell
the user that they should validate the child's constraint first.Agreed.
Patch attached. In addition to the recursion from parent case, this seems
to be broken for the alter table child inherit parent case as well. So,
fixed both MergeWithExistingConstraint (called from
AddRelationNewConstraints) and MergeConstraintsIntoExisting (called from
ATExecAddInherit). I had to add a new argument is_not_valid to the former
to signal whether the constraint being propagated itself is declared NOT
VALID, in which we can proceed with merging. Also added some tests for
both cases.It seems to work as expected and message seems to be
reasonable. Test seems to be fine.
Thanks for reviewing.
By the way I have one question.
Is it an expected configuration where tables in an inheritance
tree has different valid state on the same (check) constraint?
I would think not.
The check should be an equality if it's not.
If you mean that the valid state should be same (equal) at all times on
parent and all the child tables, then that is exactly what the patch tries
to achieve. Currently, valid state of a constraint on a child table is
left to differ from the parent in two cases as described in my messages.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello,
At Fri, 22 Jul 2016 17:35:48 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <9733fae3-c32f-b150-e368-a8f87d546a7f@lab.ntt.co.jp>
On 2016/07/22 17:06, Kyotaro HORIGUCHI wrote:
At Fri, 22 Jul 2016 14:10:48 +0900, Amit Langote wrote:
On 2016/07/22 0:38, Robert Haas wrote:
On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote wrote:
Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion. If merged, the constraint is not checked
for the child data even though it may have some. Is that an oversight?Seems like it. I'd recommend we just error out in that case and tell
the user that they should validate the child's constraint first.Agreed.
Patch attached. In addition to the recursion from parent case, this seems
to be broken for the alter table child inherit parent case as well. So,
fixed both MergeWithExistingConstraint (called from
AddRelationNewConstraints) and MergeConstraintsIntoExisting (called from
ATExecAddInherit). I had to add a new argument is_not_valid to the former
to signal whether the constraint being propagated itself is declared NOT
VALID, in which we can proceed with merging. Also added some tests for
both cases.It seems to work as expected and message seems to be
reasonable. Test seems to be fine.Thanks for reviewing.
By the way I have one question.
Is it an expected configuration where tables in an inheritance
tree has different valid state on the same (check) constraint?I would think not.
I understand that the first problem is that the problematic
state inhibits later VALIDATE CONSTRAINT on parent from working
as expected. This patch inhibits the state where a parent is
valid and any of its children is not-valid, but allows the
opposite and it is enough to fix the problem.
I thought the opposite state is ok generally but not with full
confidence.
After some reading the code, it seems to affect only on some
cache invalidation logics and constraint exclusion logic to
ignore the check constraint per component table, and
acquire_inherited_sample_rows.
The first and second wouldn't harm. The third causes needless
tuple conversion. If this is a problem, the validity state of all
relations in an inheritance tree should be exactly the same,
ether valid or not-valid. Or should make the function to ignore
the difference of validity state.
If the problem is only VALIDATE CONSTRAINT on the parent and
mixted validity states within an inheritance tree is not, making
it process whole the inheritance tree regardsless of the validity
state of the parent would also fix the problem.
After all, my concerns are the following.
- Is the mixed validity states (in any form) in an inheritnce
tree should be valid? If so, VALIDATE CONSTRAINT should be
fixed, not MergeWithExistingConstraint. If not, the opposite
state also should be inhibited.
- Is it valid to represent all descendants' validity states by
the parent's state? (Currently only VALIDATE CONSTRAINT does)
If not, VALIDATE CONSTRAINT should be fixed.
Any thoughts?
The check should be an equality if it's not.
If you mean that the valid state should be same (equal) at all times on
parent and all the child tables, then that is exactly what the patch tries
to achieve. Currently, valid state of a constraint on a child table is
left to differ from the parent in two cases as described in my messages.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello,
On 2016/07/25 12:44, Kyotaro HORIGUCHI wrote:
At Fri, 22 Jul 2016 17:35:48 +0900, Amit Langote wrote:
On 2016/07/22 17:06, Kyotaro HORIGUCHI wrote:
By the way I have one question.
Is it an expected configuration where tables in an inheritance
tree has different valid state on the same (check) constraint?I would think not.
I understand that the first problem is that the problematic
state inhibits later VALIDATE CONSTRAINT on parent from working
as expected. This patch inhibits the state where a parent is
valid and any of its children is not-valid, but allows the
opposite and it is enough to fix the problem.I thought the opposite state is ok generally but not with full
confidence.
I obviously missed the opposite case. However, it's OK for a child's
constraint to be valid while the parent's is not. There seems to be in
place only the one-way rule which is that we don't mark parent's
constraint valid until and unless it is marked valid on all the child
tables. From the following comment in ATExecValidateConstraint():
/*
* 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.
*
And it seems to be in place only for VALIDATE CONSTRAINT's purpose.
After some reading the code, it seems to affect only on some
cache invalidation logics and constraint exclusion logic to
ignore the check constraint per component table, and
acquire_inherited_sample_rows.The first and second wouldn't harm. The third causes needless
tuple conversion. If this is a problem, the validity state of all
relations in an inheritance tree should be exactly the same,
ether valid or not-valid. Or should make the function to ignore
the difference of validity state.
Hmm, perhaps check constraint valid status affecting whether
child-to-parent-rowtype conversion should occur is unnecessary. Maybe a
subset of those checks for purposes of acquire_inherited_sample_rows would
suffice. Or simply make equalTupleDescs accept a boolean parameter that
indicates whether or not to check TupleConstr equality.
If the problem is only VALIDATE CONSTRAINT on the parent and
mixted validity states within an inheritance tree is not, making
it process whole the inheritance tree regardsless of the validity
state of the parent would also fix the problem.After all, my concerns are the following.
- Is the mixed validity states (in any form) in an inheritnce
tree should be valid? If so, VALIDATE CONSTRAINT should be
fixed, not MergeWithExistingConstraint. If not, the opposite
state also should be inhibited.- Is it valid to represent all descendants' validity states by
the parent's state? (Currently only VALIDATE CONSTRAINT does)
If not, VALIDATE CONSTRAINT should be fixed.Any thoughts?
Wouldn't it be better to leave VALIDATE CONSTRAINT alone, because the way
it works now it short-circuits some extra processing if the parent's
constraint is marked valid? All we need to do is to prevent the rule from
being broken by fixing the current code like the patch proposes. If we
try to fix the VALIDATE CONSTRAINT instead, we'll always have to pay the
cost of find_all_inheritors(). Thoughts?
As for the other cases (determining whether we can live with the state
where a child's constraint is valid whereas the same on the parent and
siblings is not):
1. Both cache invalidation and acquire_inherited_sample_rows depend on
equalTupleDescs, where the former is unrelated to inheritance behavior as
such (and hence unaffected by current discussion); for the latter, we
might want to simply ignore comparing the check constraint valid status as
mentioned above
2. Constraint exclusion, where it seems OK for a child's check constraint
to cause it to be excluded while the same check constraint of its parent
and siblings is ignored causing them to be needlessly scanned.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello,
At Mon, 25 Jul 2016 15:57:00 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <fe27dd92-61bb-eb63-da47-63b535e41c48@lab.ntt.co.jp>
On 2016/07/25 12:44, Kyotaro HORIGUCHI wrote:
At Fri, 22 Jul 2016 17:35:48 +0900, Amit Langote wrote:
On 2016/07/22 17:06, Kyotaro HORIGUCHI wrote:
By the way I have one question.
Is it an expected configuration where tables in an inheritance
tree has different valid state on the same (check) constraint?I would think not.
I understand that the first problem is that the problematic
state inhibits later VALIDATE CONSTRAINT on parent from working
as expected. This patch inhibits the state where a parent is
valid and any of its children is not-valid, but allows the
opposite and it is enough to fix the problem.I thought the opposite state is ok generally but not with full
confidence.I obviously missed the opposite case. However, it's OK for a child's
constraint to be valid while the parent's is not. There seems to be in
place only the one-way rule which is that we don't mark parent's
constraint valid until and unless it is marked valid on all the child
tables. From the following comment in ATExecValidateConstraint():/*
* 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.
*And it seems to be in place only for VALIDATE CONSTRAINT's purpose.
Agreed. It guarantees nothing outside the function but it shows
that at least one side of mixed validity status is/should be
considered.
After some reading the code, it seems to affect only on some
cache invalidation logics and constraint exclusion logic to
ignore the check constraint per component table, and
acquire_inherited_sample_rows.The first and second wouldn't harm. The third causes needless
tuple conversion. If this is a problem, the validity state of all
relations in an inheritance tree should be exactly the same,
ether valid or not-valid. Or should make the function to ignore
the difference of validity state.Hmm, perhaps check constraint valid status affecting whether
child-to-parent-rowtype conversion should occur is unnecessary. Maybe a
subset of those checks for purposes of acquire_inherited_sample_rows would
suffice. Or simply make equalTupleDescs accept a boolean parameter that
indicates whether or not to check TupleConstr equality.
equalTupleDescs are used in few places. The equalTupleDescs's
"Logical" equality seems to stem from the compatibility while
given to heap_form/modify_tuple. I don't think the validity
status has something to do with the result of such functions.
If the problem is only VALIDATE CONSTRAINT on the parent and
mixted validity states within an inheritance tree is not, making
it process whole the inheritance tree regardsless of the validity
state of the parent would also fix the problem.After all, my concerns are the following.
- Is the mixed validity states (in any form) in an inheritnce
tree should be valid? If so, VALIDATE CONSTRAINT should be
fixed, not MergeWithExistingConstraint. If not, the opposite
state also should be inhibited.- Is it valid to represent all descendants' validity states by
the parent's state? (Currently only VALIDATE CONSTRAINT does)
If not, VALIDATE CONSTRAINT should be fixed.Any thoughts?
Wouldn't it be better to leave VALIDATE CONSTRAINT alone, because the way
it works now it short-circuits some extra processing if the parent's
constraint is marked valid? All we need to do is to prevent the rule from
being broken by fixing the current code like the patch proposes. If we
try to fix the VALIDATE CONSTRAINT instead, we'll always have to pay the
cost of find_all_inheritors(). Thoughts?
VALIDATE CONSTRAINT is expected to take a long time like analyze
and used for very limited cases so the cost of
find_all_inheritors() don't seem to be siginificant. However,
such modification would give a pain more than required. As the
result, I agree with you.
As for the other cases (determining whether we can live with the state
where a child's constraint is valid whereas the same on the parent and
siblings is not):1. Both cache invalidation and acquire_inherited_sample_rows depend on
equalTupleDescs, where the former is unrelated to inheritance behavior as
such (and hence unaffected by current discussion); for the latter, we
might want to simply ignore comparing the check constraint valid status as
mentioned above2. Constraint exclusion, where it seems OK for a child's check constraint
to cause it to be excluded while the same check constraint of its parent
and siblings is ignored causing them to be needlessly scanned.
Agreed to the both. So the conclusion here is,
- Inhibit only the case where parent is to be validated while
child is not-validated while merge. The oppsite is
deliberately allowed and properly handled by VALIDATE
CONSTRAINT.
That is, the last patch doesn't need modification.
- Remove ccvalid condition from equalTupleDescs() to reduce
unnecessary cache invalidation or tuple rebuilding.
The attached patch does this.
These two thing seems to be different problem.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Remove-validation-status-condition-from-equalTupleDe.patchtext/x-patch; charset=us-asciiDownload
From 1192b64496b69eaac81c1d20ce76d3f7ec2efd40 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Mon, 25 Jul 2016 16:54:35 +0900
Subject: [PATCH] Remove validation status condition from equalTupleDescs.
This function checks given two tuple descriptors generates
incompatible tuples. Whether a check constraint is validated or not
doesn't break the compatibility.
---
src/backend/access/common/tupdesc.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index b56d0e3..431eb73 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -342,10 +342,10 @@ DecrTupleDescRefCount(TupleDesc tupdesc)
/*
* Compare two TupleDesc structures for logical equality
*
- * Note: we deliberately do not check the attrelid and tdtypmod fields.
- * This allows typcache.c to use this routine to see if a cached record type
- * matches a requested type, and is harmless for relcache.c's uses.
- * We don't compare tdrefcount, either.
+ * Note: we deliberately do not check the fields attrelid, tdtypmod and
+ * ccvalid of check constraints. This allows typcache.c to use this routine
+ * to see if a cached record type matches a requested type, and is harmless
+ * for relcache.c's uses. We don't compare tdrefcount, either.
*/
bool
equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
@@ -460,7 +460,6 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
{
if (strcmp(check1->ccname, check2->ccname) == 0 &&
strcmp(check1->ccbin, check2->ccbin) == 0 &&
- check1->ccvalid == check2->ccvalid &&
check1->ccnoinherit == check2->ccnoinherit)
break;
}
--
1.8.3.1
Hello,
On 2016/07/25 17:18, Kyotaro HORIGUCHI wrote:
- Remove ccvalid condition from equalTupleDescs() to reduce
unnecessary cache invalidation or tuple rebuilding.
The following commit introduced the ccvalid check:
"""
commit c31305de5f5a4880b0ba2f5983025ef0210a3b2a
Author: Noah Misch <noah@leadboat.com>
Date: Sun Mar 23 02:13:43 2014 -0400
Address ccvalid/ccnoinherit in TupleDesc support functions.
equalTupleDescs() neglected both of these ConstrCheck fields, and
CreateTupleDescCopyConstr() neglected ccnoinherit. At this time, the
only known behavior defect resulting from these omissions is constraint
exclusion disregarding a CHECK constraint validated by an ALTER TABLE
VALIDATE CONSTRAINT statement issued earlier in the same transaction.
Back-patch to 9.2, where these fields were introduced.
"""
So, apparently RelationClearRelation() does intend to discard a cached
TupleDesc if ccvalid changed in a transaction. Whereas,
acquire_inherited_sample_rows() does not seem to depend on ccvalid being
equal or not (or any member of TupleConstr for that matter). So maybe,
instead of simply discarding the check (which does serve a purpose), we
could make equalTupleDescs() parameterized on whether we want TupleConstr
equality check to be performed or not. RelationClearRelation() can ask
for the check to be performed by passing true for the parameter whereas
acquire_inherited_sample_rows() and other callers can pass false. Perhaps
something like the attached.
Thanks,
Amit
Attachments:
equalTupDescs-compare-constr-arg-1.patchtext/x-diff; name=equalTupDescs-compare-constr-arg-1.patchDownload
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index b56d0e3..3dcb656 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -348,7 +348,7 @@ DecrTupleDescRefCount(TupleDesc tupdesc)
* We don't compare tdrefcount, either.
*/
bool
-equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
+equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2, bool compare_constr)
{
int i,
j,
@@ -411,6 +411,9 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
/* attacl, attoptions and attfdwoptions are not even present... */
}
+ if (!compare_constr)
+ return true;
+
if (tupdesc1->constr != NULL)
{
TupleConstr *constr1 = tupdesc1->constr;
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index c1d1505..7ce2da5 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -434,7 +434,7 @@ ProcedureCreate(const char *procedureName,
if (olddesc == NULL && newdesc == NULL)
/* ok, both are runtime-defined RECORDs */ ;
else if (olddesc == NULL || newdesc == NULL ||
- !equalTupleDescs(olddesc, newdesc))
+ !equalTupleDescs(olddesc, newdesc, false))
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 5fcedd7..f96333d 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1417,7 +1417,8 @@ acquire_inherited_sample_rows(Relation onerel, int elevel,
/* We may need to convert from child's rowtype to parent's */
if (childrows > 0 &&
!equalTupleDescs(RelationGetDescr(childrel),
- RelationGetDescr(onerel)))
+ RelationGetDescr(onerel),
+ false))
{
TupleConversionMap *map;
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index f42a62d..d3b0d6c 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -711,7 +711,7 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
/* OK, doesn't return tuples */
}
else if (resultDesc == NULL || plansource->resultDesc == NULL ||
- !equalTupleDescs(resultDesc, plansource->resultDesc))
+ !equalTupleDescs(resultDesc, plansource->resultDesc, false))
{
/* can we give a better error message? */
if (plansource->fixed_result)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 8d2ad01..228f1f8 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2240,7 +2240,7 @@ RelationClearRelation(Relation relation, bool rebuild)
elog(ERROR, "relation %u deleted while still in use", save_relid);
}
- keep_tupdesc = equalTupleDescs(relation->rd_att, newrel->rd_att);
+ keep_tupdesc = equalTupleDescs(relation->rd_att, newrel->rd_att, true);
keep_rules = equalRuleLocks(relation->rd_rules, newrel->rd_rules);
keep_policies = equalRSDesc(relation->rd_rsdesc, newrel->rd_rsdesc);
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index ea6f787..b4578f5 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -1341,7 +1341,7 @@ assign_record_type_typmod(TupleDesc tupDesc)
foreach(l, recentry->tupdescs)
{
entDesc = (TupleDesc) lfirst(l);
- if (equalTupleDescs(tupDesc, entDesc))
+ if (equalTupleDescs(tupDesc, entDesc, false))
{
tupDesc->tdtypmod = entDesc->tdtypmod;
return;
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index de18f74..28a85f4 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -110,7 +110,7 @@ extern void DecrTupleDescRefCount(TupleDesc tupdesc);
DecrTupleDescRefCount(tupdesc); \
} while (0)
-extern bool equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
+extern bool equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2, bool compare_constr);
extern void TupleDescInitEntry(TupleDesc desc,
AttrNumber attributeNumber,
Hello,
At Mon, 25 Jul 2016 18:21:27 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <96fb8bca-57f5-e5a8-9630-79d4fc5b213e@lab.ntt.co.jp>
Hello,
On 2016/07/25 17:18, Kyotaro HORIGUCHI wrote:
- Remove ccvalid condition from equalTupleDescs() to reduce
unnecessary cache invalidation or tuple rebuilding.The following commit introduced the ccvalid check:
"""
commit c31305de5f5a4880b0ba2f5983025ef0210a3b2a
Author: Noah Misch <noah@leadboat.com>
Date: Sun Mar 23 02:13:43 2014 -0400Address ccvalid/ccnoinherit in TupleDesc support functions.
equalTupleDescs() neglected both of these ConstrCheck fields, and
CreateTupleDescCopyConstr() neglected ccnoinherit. At this time, the
only known behavior defect resulting from these omissions is constraint
exclusion disregarding a CHECK constraint validated by an ALTER TABLE
VALIDATE CONSTRAINT statement issued earlier in the same transaction.
Back-patch to 9.2, where these fields were introduced.
"""
Wow.. Missed the obvious thing. Certainly relation cache must be
invalidated by a change of ccvalidated as the commit message.
So, apparently RelationClearRelation() does intend to discard a cached
TupleDesc if ccvalid changed in a transaction. Whereas,
acquire_inherited_sample_rows() does not seem to depend on ccvalid being
equal or not (or any member of TupleConstr for that matter). So maybe,
instead of simply discarding the check (which does serve a purpose), we
could make equalTupleDescs() parameterized on whether we want TupleConstr
equality check to be performed or not. RelationClearRelation() can ask
for the check to be performed by passing true for the parameter whereas
acquire_inherited_sample_rows() and other callers can pass false. Perhaps
something like the attached.
Hmm. It should resolve the problem. But the two comparisons seem
to be separate things. Constraints is not a part of tuple
description. relcache seems to be making misusage of the equality
of tuple descriptors.
So, how about splitting the original equalTupleDescs into
equalTupleDescs and equalTupleConstraints ?
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Split-equalTupleDescs-into-two-functions.patchtext/x-patch; charset=us-asciiDownload
From 5a1d570ac327702fea19ed18198efeb02632a2f3 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Tue, 26 Jul 2016 11:02:48 +0900
Subject: [PATCH] Split equalTupleDescs into two functions.
equalTupleDescs intends to the two given tuple descriptors are
compatible on generating a tuple. Comaprison on constraints are
required to invalidate cache but it is a separate matter. So split it
into two new functions equalTupleDescs and equalTupleConstraints.
---
src/backend/access/common/tupdesc.c | 14 ++++++++++++++
src/include/access/tupdesc.h | 1 +
2 files changed, 15 insertions(+)
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index b56d0e3..1e42c87 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -411,6 +411,19 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
/* attacl, attoptions and attfdwoptions are not even present... */
}
+ return true;
+}
+
+/*
+ * Compare constraints between two TupleDesc structures
+ */
+bool
+equalTupleConstraints(TupleDesc tupdesc1, TupleDesc tupdesc2)
+{
+ int i,
+ j,
+ n;
+
if (tupdesc1->constr != NULL)
{
TupleConstr *constr1 = tupdesc1->constr;
@@ -470,6 +483,7 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
}
else if (tupdesc2->constr != NULL)
return false;
+
return true;
}
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index de18f74..58fa466 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -111,6 +111,7 @@ extern void DecrTupleDescRefCount(TupleDesc tupdesc);
} while (0)
extern bool equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
+extern bool equalTupleConstraints(TupleDesc tupdesc1, TupleDesc tupdesc2);
extern void TupleDescInitEntry(TupleDesc desc,
AttrNumber attributeNumber,
--
1.8.3.1
Hello,
On 2016/07/26 11:05, Kyotaro HORIGUCHI wrote:
At Mon, 25 Jul 2016 18:21:27 +0900, Amit Langote wrote:
So, apparently RelationClearRelation() does intend to discard a cached
TupleDesc if ccvalid changed in a transaction. Whereas,
acquire_inherited_sample_rows() does not seem to depend on ccvalid being
equal or not (or any member of TupleConstr for that matter). So maybe,
instead of simply discarding the check (which does serve a purpose), we
could make equalTupleDescs() parameterized on whether we want TupleConstr
equality check to be performed or not. RelationClearRelation() can ask
for the check to be performed by passing true for the parameter whereas
acquire_inherited_sample_rows() and other callers can pass false. Perhaps
something like the attached.Hmm. It should resolve the problem. But the two comparisons seem
to be separate things. Constraints is not a part of tuple
description. relcache seems to be making misusage of the equality
of tuple descriptors.So, how about splitting the original equalTupleDescs into
equalTupleDescs and equalTupleConstraints ?
Actually TupleConstr is *part* of the TupleDesc struct, which the
relcache.c tries to preserve in *whole* across a relcache flush, so it
seems perhaps cleaner to keep the decision centralized in one function:
keep_tupdesc = equalTupleDescs(relation->rd_att, newrel->rd_att);
...
/* preserve old tupledesc and rules if no logical change */
if (keep_tupdesc)
SWAPFIELD(TupleDesc, rd_att);
Also:
/*
* This struct is passed around within the backend to describe the
* structure of tuples. For tuples coming from on-disk relations, the
* information is collected from the pg_attribute, pg_attrdef, and
* pg_constraint catalogs.
...
typedef struct tupleDesc
{
It would perhaps have been clear if there was a rd_constr field that
relcache.c independently managed.
OTOH, I tend to agree that other places don't quite need the whole thing
(given also that most other callers except acquire_inherit_sample_rows
compare transient row types)
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
At Tue, 26 Jul 2016 13:51:53 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <f8f2a32e-46da-d34d-cabc-24e75d6da082@lab.ntt.co.jp>
So, how about splitting the original equalTupleDescs into
equalTupleDescs and equalTupleConstraints ?Actually TupleConstr is *part* of the TupleDesc struct, which the
relcache.c tries to preserve in *whole* across a relcache flush, so it
seems perhaps cleaner to keep the decision centralized in one function:
The "Logical" is the cause of the ambiguity. It could be thought
that relation cache maintains "Physical" tuple descriptor, which
is defferent from "Logical" one.
keep_tupdesc = equalTupleDescs(relation->rd_att, newrel->rd_att);
...
/* preserve old tupledesc and rules if no logical change */
if (keep_tupdesc)
SWAPFIELD(TupleDesc, rd_att);Also:
/*
* This struct is passed around within the backend to describe the
* structure of tuples. For tuples coming from on-disk relations, the
* information is collected from the pg_attribute, pg_attrdef, and
* pg_constraint catalogs.
...
typedef struct tupleDesc
{It would perhaps have been clear if there was a rd_constr field that
relcache.c independently managed.OTOH, I tend to agree that other places don't quite need the whole thing
(given also that most other callers except acquire_inherit_sample_rows
compare transient row types)
Yes, constraints apparently doesn't affect the shpae of
generatred tuples. On the other hand relcache should be aware of
changes of constraints. Furthermore the transient row types has
utterly no relation with constraints of even underlying
relations.
So, almost as your proposition, what do you think if the
equalTupleDescs has extra parameter but named "logical", and
ignore constraints if it is true? (Obviously the opposite will
do). What I felt uneasy about is the name "compare_constr".
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jul 22, 2016 at 1:10 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2016/07/22 0:38, Robert Haas wrote:
On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion. If merged, the constraint is not checked
for the child data even though it may have some. Is that an oversight?Seems like it. I'd recommend we just error out in that case and tell
the user that they should validate the child's constraint first.Agreed.
Patch attached. In addition to the recursion from parent case, this seems
to be broken for the alter table child inherit parent case as well. So,
fixed both MergeWithExistingConstraint (called from
AddRelationNewConstraints) and MergeConstraintsIntoExisting (called from
ATExecAddInherit). I had to add a new argument is_not_valid to the former
to signal whether the constraint being propagated itself is declared NOT
VALID, in which we can proceed with merging. Also added some tests for
both cases.
Well, on second thought, I'm no longer sure that this approach makes
sense. I mean, it's obviously wrong for constraint-merging to change
the validity marking on a constraint, but that does not necessarily
imply that we shouldn't merge the constraints, does it? I see the
downthread discussion saying that it's a problem if the parent's
constraint is marked valid while the child's constraint isn't, but I
don't quite understand why that situation would cause trouble. In
other words, I see that the current situation is not good, but I'm not
sure I understand what's going on here well enough to be confident
that any of the proposed fixes are correct.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
Well, on second thought, I'm no longer sure that this approach makes
sense. I mean, it's obviously wrong for constraint-merging to change
the validity marking on a constraint, but that does not necessarily
imply that we shouldn't merge the constraints, does it? I see the
downthread discussion saying that it's a problem if the parent's
constraint is marked valid while the child's constraint isn't, but I
don't quite understand why that situation would cause trouble. In
other words, I see that the current situation is not good, but I'm not
sure I understand what's going on here well enough to be confident
that any of the proposed fixes are correct.
The point I think is that a valid CHECK constraint on a parent table
should imply that all rows fetched by "SELECT * FROM table" will pass
the check. Therefore, a situation with valid parent constraint and
not-valid child constraint is bad because it might allow some rows
fetched by an inheritance scan to not pass the check. Quite aside from
any user-level expectations, this could break planner optimizations.
I'd be satisfied with the upthread proposal "throw error if the child has
a matching not-valid constraint". Allowing the merge if both child
and new parent constraint are not-valid is all right as an extension,
but it seems like a feature with a mighty narrow use case, and I would
not go far out of our way to support it. Causing the command to not
merge but instead create a new duplicate child constraint seems like a
seriously bad idea (though I'm not sure that anyone was advocating for
that).
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers