Thread: 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
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
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
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
Attachment
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
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
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
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
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 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. 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 deliberatelyallowed 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
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
Attachment
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 -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. > """ 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
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
logicalchange */ 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-diskrelations, 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
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
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
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