Seawolf, I think that answer is not to the question. Can you please think again about the question? Would passing the parameter as 'var' be useful to avoid these AV's without causing a new memory leak?
Olehs, did you try your patch and did it solve the AV errors later on?
I'd like to share some notices about the patch. It works OK if connection is used for single dataset object. That way on exception it frees the connection on pqlib and on statement. The only problem of that is that if someone tries to ExecSql statement with Handle=nil there is no exception risen. It seems if the connection was recovered... Of course it is not.
The real problem arises then there are several data sets and several Statement objects. As only one Statement set Handle:=nil - the others still try to use their own handles that are not nil (and gets access violation).
This is because the connection handle is stored on statement class. Setting FHandle := nill does not affect other statement objects created earlier and even after getting exception of broken connection.
As I understand - the connection handle should only be stored on Connection class. Other classes should get this handle via get method. TZPostgreSQLConnection has method function GetConnectionHandle: PZPostgreSQLConnect so why not use it?
The other problem - what to do to TZConnection object after connection is lost? Setting it to disconnected mode would seem the answer, but that would brake the backward compatibility as some people may have their way to deal with such problems. Disconnecting the connection would close all its dataset objects and so change the current behavior of programs.
Also I'm not entirely sure, that PlainDriver.Finish(Handle) should be called at all. May someone know why it appeared there at all? Why can't we leave libpq in disconnected state until user decides to close the connection or to reset it?... I don't see how the memory leak should appear there as someone would have to close the connection after all...
What is the practice of dealing with broken connections while working with other DB's ?
I upload patches:
1. I removed the connection closing code from CheckPostgreSQLError as this is not in place (imho). Connection is finished when tconnection closes. Also I check if connection is active to know if rollback should be attempted. I removed the var directive from Handle parameter. This is not needed anymore. I think handle parameter should go away to, but let's leave it for now.
You do not have the required permissions to view the files attached to this post.
I removed FHandle from Statement Classes. Handle is retrieved using GetConnectionHandle procedure. I think storing connection handle in every statement was wrong.
You do not have the required permissions to view the files attached to this post.
the connection handle should only be stored on Connection class. Other classes should get this handle via get method. TZPostgreSQLConnection has method function GetConnectionHandle: PZPostgreSQLConnect so why not use it?
That sounds like elegant solution. I'll try your patches.
what to do to TZConnection object after connection is lost?
Most database applications that I encounter just shows nasty AV error after lost connection and leave end user to handle it (read: send screen shot / complaint to administrator). And this behaviour is not "privilege" of zeos based apps.
Maybe a good way to solve this is introduction TZConnection.OnConnectionLost event handler (or something similar)?
Also I'm not entirely sure, that PlainDriver.Finish(Handle) should be called at all.
Oh, definitely should be, for internal libpq cleanup. Postgres documentation is quite clear about that.
Oh, definitely should be, for internal libpq cleanup. Postgres documentation is quite clear about that.
I have no doubt PlainDriver.Finish(Handle) should be called. I'm just not sure if it should be called in CheckPostgreSQLError. As handle is a property of connection object closing the connection shout be (and is) closing the handle. What CheckPostgreSQLError could do on connection break is IConnection.Close, but that would break the backward compatibility as closing the connection closes all the datasets linked to it. Reconnecting after that would be a pain. I don't think such change would make happy many developers...
To start with I'll say I'm referring to PostgreSQL 8.4.2 Documentation: 30.1. Database Connection Control Functions when talking of libpq. It basically says: "Note that if PQconnectStart returns a non-null pointer, you must call PQfinish when you are finished with it, in order to dispose of the structure and any associated memory blocks. This must be done even if the connection attempt fails or is abandoned."
This part is clear - "Everything that has a beginning has an end, Neo" (The matix). I failed to find any orders to finish the handle on some status or error received. Now we have to agree when the handle should be finished. As I understand the best way is to have one place to create something and to finish. If on TZConnection.Connect PGconn object is created then the most natural time to destroy it should be on Disconnect. This way it is right now. After a connection break handle remains valid and stays that way until the connection is closed. Any program after "Connection lost" Exception can either try to reconnect or close the connection and close. I'm not sure now, but I hope on TZConnection destruction it disconnects so the handle is finished then and I can't see any memory leakage there...
Now Finishing the handle on connection loss is a different story. We give up the handle that was created on Connect, but the TZConnection object stays connected. That way before executing any statement we should check the handle and if it is nil then inform the user. We should check the handle on disconnecting to see if we can finish it. So I see that finishing the handle on PG error makes so many troubles it is not worth it. Not to mention it does't do any good.
That's what I thing. If I missed something in Postgres documentation or in Zeos code - please let me know.
Thanks for your study and elaborate explanations. I agree there's some inconsistency when we set the handle to nil when the connection breaks unexpectedly. But now we get the handle from the connection directly we can also set the connection handle to nil and check for it in the GetHandle function, raising an exception at that time (even silently trying to reconnect including OnReconnect handling may be an option if pg supports it) This would at least prevent some nasty AV's.
I agree that this can be done. The question is - should it?... There are no orders in pglib documentation that handle should be freed after connection loss, there are no "nasty AV's" if we don't finish the handle on connection loss (libpq knows it lost connection and you get "nice" error message - not exception or AV) and finally I don't see where is the memory leakage because of which the finish was added to CheckPostgreSQLError at first place. We can add many fancy checks but that will lead to nothing better than it is when we remove the finish from CheckPostgreSQLError. And as my colleague used to say "the source code should be short - otherwise the programmer will not have time for beer". I agree to him and say "keep it simple and keep it working". Let's not write a single line of code if it doesn't make the program better.
