Re: [rt.cpan.org #81516] Test failures due to hash randomisation in perl 5.17.6

On 12.12.12 20:34, H.Merijn Brand wrote:
> On Wed, 12 Dec 2012 16:31:44 +0100, Jens Rehsack <[email protected]>
> wrote:
>
>> On 01.12.12 23:14, Tim Bunce wrote:
>>> On Thu, Nov 29, 2012 at 10:15:19PM +0100, Jens Rehsack wrote:
>>>>
>>>> But back to the issue - now it seems dbm_tables is fetched earlier
>>>> in some cases than f_dir which causes an invocation of
>>>> DBI::DBD::SqlEngine::Table::get_table_meta (including
>>>> DBI::DBD::SqlEngine::Table::bootstrap_table_meta) before f_dir
>>>> has been set. This causes $dbh->{sql_meta}->{fred}->{f_dir} being
>>>> initialized to the default value of $dbh->{f_dir} which is
>>>> always cur_dir().
>>>>
>>>> Looking into DBI::DBD::SqlEngine::dr::connect around line 180
>>>> it could be seen that there's already some magic for "some
>>>> settings must be done before others".
>>>>
>>>> A quick fix for "now" could be: keys: ("sql_meta", $dbh->{dbm_meta},
>>>> ...) must be initialized last - after any other k/v pair from %$attrs.
>>>> Another quick should could be forbid setting meta info during connect(),
>>>> as it's documented - but this would be a hugh step backwards in my
>>>> effort making DBI::DBD::SqlEngine and derived DBD's usable through
>>>> Gofer proxy. So I'd prefer the first quick shot ...
>>>>
>>>> For longer way, I'd like to refactor the order procedure to a
>>>> more flexible way:
>>>> $dbh->{dbd_init_order} = [
>>>>     [qw(Profile RaiseError PrintError AutoCommit)],
>>>>     [qw(ReadOnly ...)],
>>>>     ...
>>>>     [qw(sql_meta dbm_tables ...)]
>>>> ];
>>>>
>>>> Remaining question in that proposal: where's the fence between "must
>>>> be last" and "insert unnamed attrs here"?
>>>
>>> I don't know, but I would like a working DBI for 5.17.6+ before too long.
>>
>> Well, we developed two patches for now:
>>
>> 1) Merijn's patch - http://pasta.test-smoke.org/385
>>
>> + looks simple, easy to understand
>    + easy to extend
>    + easy to override

This is a good and bad point in once: easy to override allows
easy modifying the order, but it allows modifying the order
easily ;) This can cause in hard to debug errors like we got
one in RT#81516. And put a warning on it: use only if you
completely understood DBI::DBD::SqlEngine and DBD::File internals
is similar to: "don't do"

>    + one single point of maintenance

Which ignores the design of DBI::DBD::SqlEngine which forces
a separate place for those kind of initializations:
DBD::Your_Drv::db::init_default_attributes.

>> - makes assumptions about attribute names of derived DBD's
>>     (e.g. dbm_tables, but this can be easily renamed by assigning
>>      new name for it to $dbh->{dbm_meta} (by driver author), eg.
>>      $dbh->{dbm_meta} = "dbm_sources")
>      which makes me/us/you want more documentation about what DBI/DBD
>      guarantees, expects, supports etc. there is no real "you should do
>      this" or "this is officially off-bound"

Did you read DBD::File::Developers (and DBI::DBD::SqlEngine::Developers)?

I agree, those documents need to be extended, but many of the
things I tell are described there.

>> - increases complexitiy when more issues came up with similar
>>     patterns (eg. /_meta$/ can't be set from outside, but from
>>     derived driver)
>      but still offers easy documentation for every single entry
>
>> 2) Jens' patch - http://pasta.test-smoke.org/387
>>
>> + backward compatible to ancient attributes (csv_file, csv_ext, ...)
>      though I value this point, we are talking about DBD's we all
>      control. The newest DBD::CSV and DBD::DBM will require the new DBI
>      so conflicts are not likely to happen. To be honest, I - as
>      maintainer of DBD::CSV - didn't even know csv_file was still
>      supported

My fault, I thought it would. But DBD::File supports dbm_ext (aliases
f_ext) as well as dbm_lockfile (aliases f_lockfile). I'm pretty sure,
DBD::AnyData (greetings to Sven Doweit) will have similar surprises.

>> + no assumptions on derived DBD's
>      a very good point
>> + no false positives on pattern match
>      if everything is well-documented, then false positives should not
>      happen or better cause havoc
>> - more complicated for quick review
>      can be solved by well-chosen inline docs
>    - This patch will probably need to have extra code in (many) other
>      places, which my patch tried to avoid

I wouldn't expect - I see only one extra point which is in DBD::File
to enable backward compatibility to DBD::File 0.40 for at least
developers (aliasing f_meta to sql_meta). Skipping this, even DBD::File
needs no extra code.

To value your comments, I've reworked my patch a little bit to allow
easier modifying in sub-classes (derived DBD's).
You can find it at http://pasta.test-smoke.org/389

>> Any other +/- comments?
>
> Yes please, don't feel shy!
> Tell us!
>
>> In general both versions would fix the root cause of RT#81516,
>> while the maintainers (Merijn, me) cannot choose the right one.
>
> What he said
>
>> Jens

/o\

Jens