Patch for DBIx::Connector::Driver::Oracle #25

Closed
byterock opened this Issue Oct 31, 2012 · 4 comments

Projects

None yet

2 participants

You should just use the ping() method for this as that will use a good deal less resources than a do-->select and you do not need the eval and the in line if,

sub ping {
my ($self, $dbh) = @_;
-- eval {
-- local $dbh->{RaiseError} = 1;
-- $dbh->do('select 1 from dual');
-- };
-- return $@ ? 0 : 1;
++ return $dbh->ping();
}

This will do 1 round trip (if you have an older DBD::Oracle and or Oracle client it may do more) , I think this has been implemented on a number of other DBDs as well so you might want to check the others

Owner
theory commented Oct 31, 2012

I borrowed that code from DBIx::Class. I don't know the reason for it, other than that ping used to not work right. Not sure if that's because of a bug in Oracle or a bug in DBD::Oracle, or when (or if!) it has been fixed. I'm happy to make this change, but if it was fixed at a certain point, I'd rather have it do the actual SELECT for broken versions of Oracle or DBD::Oracle, as appropriate.

Owner
theory commented Oct 31, 2012

The reason for this workaround is described in this comment from @ribasushi.

byterock commented Nov 1, 2012

Hmm I had a good look at the code since DBD::Oracle 1.22 we have been using 'OCIServerVersion' later in 1.25 we added OICPing for Oracle clients >10.2

I could se how 'OCIServerVersion' could give you a false connection result as it does not really do with OCIPing does. Never did see that as a bug with DBD::Oracle on th bug list or mailing list.

So what I will do it find another call than 'OCIServerVersion' and add that to DBD::Oracle

What I can say if you are using DBD::Oracle greter than1.25 and an Oracle client greater than 10.2 they you would always be using OCIPing so it will work for those cases. DBD::Oracle's less than 1.22 would work as well as they use a DBD->do as well.

So 1.22 to 1.24 will have the bug and any time the Oracle client is less than 10.2
cheers
John

Owner
theory commented Mar 20, 2013

Okay, I think the consensus from the discussion was to leave it as-is. So closing this issue.

@theory theory closed this Mar 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment