Re: Review: Non-inheritable check constraints

Hi Alex,

> Hmmm, your patch checks for a constraint being "only" via:
> >
> > !recurse && !recursing
> >
> > I hope that is good enough to conclusively conclude that the constraint
> is
> > 'only'. This check was not too readable in the existing code for me
> anyways
> > ;). If we check at the grammar level, we can be sure. But I remember not
> > being too comfortable about the right position to ascertain this
> > characteristic.
>
> Well I traced through it here was my thinking (maybe should be a comment?):
>
> 1: AlterTable() calls ATController() with recurse =
> interpretInhOption(stmt->relation->inhOpt
> 2: ATController() calls ATPrepCmd() with recurse and recursing = false
> 3: ATPrepCmd() saves the recurse flag with the subtup
> "AT_AddConstraintRecurse, otherwise the subtype is AT_AddConstraint
> 4: ATExecCmd() calls ATExecAddConstraint() with recurse == true when
> subtype == AT_AddConstraintRecurse, recurse = false otherwise
> 5: ATExecAddConstraint() calls ATAddCheckConstraint() with recuse and
> recursing = false
> 6: now we are in ATAddCheckConstraint() where recurse ==
> interpretInhOption(rv->inhOpt) and recursing == false. Recursing is
> only true when ATAddCheckConstaint() loops through children and
> recursively calls ATAddCheckConstraint()
>
> So with it all spelled out now I see the "constraint must be added to
> child tables too" check is dead code.
>
>
Thanks the above step-wise explanation helps.

But AFAICS, the default inhOpt value can be governed by the SQL_inheritance
guc too. So in that case, it's possible that recurse is false and child
tables are present, no?

Infact as I now remember, the reason my patch was looping through was to
handle this very case. It was based on the assumptions that some constraints
might be ONLY type and some can be inheritable. Although admittedly the
current ALTER TABLE functionality does not allow this.

So maybe we can still keep this check around IMO.

Regards,
Nikhils