[Dbix-class] SQL::Translator::Parser::DBIx::Class unique symbol change

[luke saunders]

On Thu, May 15, 2008 at 6:53 PM, Ryan D Johnson <ryandjohnson at gmail.com> wrote:
> "John Goulah" <jgoulah at gmail.com> writes:
>
>> On Thu, May 15, 2008 at 9:40 AM, luke saunders <luke.saunders at gmail.com> wrote:
>>> On Thu, May 15, 2008 at 2:24 PM, Ash Berlin <ash_cpan at firemirror.com> wrote:
>>>>
>>>> On 15 May 2008, at 14:08, luke saunders wrote:
>>>>>
>>>>> What were these cases? Is that not just a bug with the unique symbol
>>>>> generator in the producer?
>>>>
>>>> I'm not sure of the exact cause any more, but I think it was to do with
>>>> multi-col unique constraints and FKs - i.e. if you have a unique across two
>>>> cols, and one of those is a FK constraint also, it would just barf - the 2nd
>>>> and 3rd changes in the diff. Previously it was just using the first column
>>>> name, which caused problems.
>>>>
>>>
>>> So presumably it was barfing because the names were the same right?
>>> Which would be fixed if the producer just named the duplicate '*_2'.
>>> It already does that with FK names so shouldn't be hard to enforce the
>>> same rule globally.
>>>
>>
>> When I helped rdj through fixing this, I encouraged him to keep
>> backward compatibility which he didn't do.   I'd say breaking backward
>> compat is a -really bad idea-  for large production databases.
>> Dropping and re-adding constraints is not a fast operation on large
>> tables.
>
> Hey, guys.
>
> The specific problem with the old naming code was that it was
> unstable. I might have a constraint foo today which tomorrow the sqlt
> layer decided to name foo_2 (because there's a new constraint it wants
> to name foo). This is bad for exactly the same reasons that changing
> from the old naming code to the new naming code sucks.
>
> It's not clear that there's really a way to provide backwards
> compatibility, given that the old naming scheme was unstable. I am happy
> to provide a patch that enables a selection between the old unstable
> naming scheme and the new stable one.
>
> But we need either a leadership decision or community consensus on the
> default naming scheme. To sum up:
>
> Should we tax old users by requiring them to opt-out of the new naming
> or tax all future users by given them an unstable naming scheme by
> default?
>
> I vote in my own very biased way to use my code by default. My reasoning
> is that it's highly unlikely that a new user would think to opt-in to
> the new stable naming scheme unless or until they independently
> discovered the instability for themselves. On the other hand, when an
> old user upgrades and we try to drop and recreate all the constraints
> (like with Luke -- sorry!), I'd say they're very motivated to find the
> POD and discover the switch to turn off the new code.
>
> Sorry to cause so much trouble. I hope my reasoning for changing the
> naming scheme is now clear. I welcome feedback as to the issue of
> default behavior going forward.

It's true that it's undesirable to have foo jump to foo_2 and so I
suppose the new code is a reasonable way to minimise the likelyhood of
that. So I agree, it should be the default behaviour.

However, in our and presumably some other cases we still need to keep
the old names to prevent the add/drop situation. I am undecided
whether it's best to do this using a new rel attribute called
'constraint_name' or something which the parser uses as the name if
present or if it's best to just use a switch. The param seems more
useful generally while the switch is easier to use. I'm leaning
towards the param as with the switch there's the chance that we might
hit the same problem you did in the future and end up having to switch
to the new behaviour anyway. Anyone else have an opinion on this?

The other thing that gets me about this patch is that the max symbol
length isn't something that's specific to the DBIC parser but more the
target database, so should reside in the producers. That way the
producers can set the max lengths to whatever they require, if
anything.

Cheers,
Luke.