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