search

mattn / go-oci8

Oracle driver for Go using database/sql
https://mattn.kaoriya.net/
MIT License
630 stars 212 forks source link

Handle OCI_SUCCESS_WITH_INFO on Pings #330

Closed stampy88 closed 5 years ago

stampy88 commented 5 years ago
MichaelS11 commented 5 years ago

@mattn Seems fine with me.

MichaelS11 commented 5 years ago

@mattn Side note, looks like wnameless/oracle-xe-11g was removed. I assume Oracle is going after sites and removing these images?

stampy88 commented 5 years ago

It looks like there may be more cases where this fails too. I am going to include them as well in few minutes.

stampy88 commented 5 years ago

@mattn I think this brings up a bigger question: is there anytime where you would want OCI_SUCCESS_WITH_INFO to be handled as an error and not success? Looking through the code base there are lots of checks for OCI_SUCCESS.

stampy88 commented 5 years ago

Added checking in the OCI8DriverStruct.Open as well, and that got me working, e.g. selects, updates, inserts, deletes. My original question above still stands though. This may be something that should be tackled across the project.

mattn commented 5 years ago

@MichaelS11

Side note, looks like wnameless/oracle-xe-11g was removed. I assume Oracle is going after sites and removing these images?

Hmm, I have no idea. :(

I think this brings up a bigger question: is there anytime where you would want OCI_SUCCESS_WITH_INFO to be handled as an error and not success? Looking through the code base there are lots of checks for OCI_SUCCESS.

Yes, I think so.

MichaelS11 commented 5 years ago

I probably would not add OCI_SUCCESS_WITH_INFO to everything. Honestly would really like to understand more why it is happening for the 3 locations so far in this PR.

That being said, I am fine with changing the 3 locations to have OCI_SUCCESS_WITH_INFO, just would not add any more at this time.

stampy88 commented 5 years ago

@MichaelS11 I was hoping you guys could give a better idea why it is only needed in those 3 places. And honestly, it is only needed in 2 of the 3 since this is always true

https://github.com/mattn/go-oci8/blob/master/oci8.go#L245

I won't add anymore, so merge away if you guys are happy.

stampy88 commented 5 years ago

@MichaelS11 any more thoughts on this?

MichaelS11 commented 5 years ago

Was curious, do you know of any test cases for these?

stampy88 commented 5 years ago

The way I happened to see this was that we spun up a new DB, but that password was set to expire after a certain period. Once we got close to the expiration date, a warning started coming back and the code wasn't handling it.

MichaelS11 commented 5 years ago

Ah, I see now. Got it.

@mattn Looks good to me.

mattn commented 5 years ago

Thank you