On Thu, Oct 6, 2011 at 02:42, Nikhil Sontakke <nikkhils@gmail.com> wrote:
> Hi Alex,
>
>> I didn't care for the changes to gram.y so I reworked it a bit so we
>> now pass is_only to AddRelationNewConstraint() (like we do with
>> is_local). Seemed simpler but maybe I missed something. Comments?
>>
>
> 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.
Ill take out that check and then mark it as ready for commiter (and
Ill add any comments about the logic of the recurse flag above if I
can think of a concise way to put it). Sound good?
