TZPgEventAlerter and connection problems

[Wild_Pointer]

I've noticed, that TZPgEventAlerter does not detect seerver disconnects. This seems like a big issue as you can't trust the OnNotify event to be fired. The only way would be to periodically execute SQL statements to see if connection still active.
The cause was there was no handling of error returned by ConsumeInput.

Attaching patch ZPgEventAlerter.txt to fix the problem (had to change extension .diff to .txt, diff seems to be considered invalid extension in the forum).
As Notification reading is done using timer events there is no simple way to handle exceptions. There is no where to put try ... except block in application using TZPgEventAlerter. You could use TApplicationEvents of course but that does not seem to be intuitive use case. For that reason I've added a OnError event.

Feel free to comment. Any feedback is most welcome.

[Fr0sT]

There's a;ready ZIBEventAlerter unit with error handling so the change should be done accordingly.
Some code style suggestions:
- const ErrorMessage in TZPgErrorEvent. In 7.3 all ref-counted types in parameters made const or var to avoid excess ref changing.
- FOnError instead of FErrorHandler (why use different name?)
- You do no conversion between PAnsiChar ErrorMsg and String ErrorMessage, with D2009+ there will be warning about implicit conversion; moreover, the DB and native codepages must be considered. Look at ZDbcPostgreSqlUtils.CheckPostgreSQLError function.

And for others to think: ZIBEventAlerter.TEventAlert/TErrorEvent should be TZIBEventAlert/TZIBErrorEvent, aren't they? But this would be a breaking change. OTOH, this change is easy to track and correct (build error => examine ZIBEventAlerter unit => find changed identifiers => correct the code). Maybe we could compromise: for 7.2 add TZIB* declarations and mark old ones as deprecated while for 7.3 just change declarations.

[marsupilami]

Hello Wild_Pointer, Fr0st,

(had to change extension .diff to .txt, diff seems to be considered invalid extension in the forum).

I added diff to be an allowed extension now.

Regarding the patch: I suggest to use CheckPostgreSqlError from ZDBCPostgreSqlUtils.pas to check for errors. It does several things at once: It creates an exception, if there is an error. It also notifies the driver if it detects a connection loss. It already contains code for the correct character set conversion from PostgreSQL to Zeos. It logs the error to the TZSQLMonitor component. And last but not least it generates an exception, which allows throwing a proper exception if no error handler is installed.

Regarding the structure of TZPgErrorEvent: I suggest to add a boolean parameter with the name handled or isHandled. So maybe the code should look something like this:

Code: Select all

  if PlainDRV.ConsumeInput(Handle)<>1 then
  try
    try
      CheckPostgreSQLError(...);
    except
      on E: EZSQLException do begin
        isHandled := false;
        if Assigned(FOnError) then FOnError(self, E.Message, isHandled);
        if not isHandled then raise;
      end;
    end;
  finally
    while True do
    begin
      Notify := PlainDRV.Notifies(Handle);
      if Notify = nil then
        Break;
      HandleNotify(Notify);
      PlainDRV.FreeNotify(Notify);
    end;  
  end;

ZIBEventAlerter.TEventAlert/TErrorEvent should be TZIBEventAlert/TZIBErrorEvent, aren't they? But this would be a breaking change. OTOH, this change is easy to track and correct (build error => examine ZIBEventAlerter unit => find changed identifiers => correct the code). Maybe we could compromise: for 7.2 add TZIB* declarations and mark old ones as deprecated while for 7.3 just change declarations.

I agree.

Also I don't like the way errors are handeled by TZIBEventAlerter. TIBEventThread.SQueEvents uses an exception. But it only queues an error code and not the exception message. I don't see a good reason to not queue the error message. Another thing, we could change in Zeos 7.3. Or we reagrd it as a bug and also do that on Zeos 7.2...

Opinions?

[Fr0sT]

if not isHandled then raise;

I'm not sure that raising exceptions in OnTimer is a good idea...

Also I don't like the way errors are handeled by TZIBEventAlerter. TIBEventThread.SQueEvents uses an exception. But it only queues an error code and not the exception message. I don't see a good reason to not queue the error message. Another thing, we could change in Zeos 7.3. Or we reagrd it as a bug and also do that on Zeos 7.2...

Opinions?

Actually I examined IBX code and it seems Zeos' ZIBEventAlerter.pas code is almost 100% copy of IBEvents.pas... o_O I'm really unsure this is OK without even a note in unit header. Probably both versions are inherited from FIB but I don't know the truth.

Looking at the code I'd say it is very weird.

Code: Select all

    try
    Status := Parent.PlainDriver.isc_que_events(StatusVector,
      Parent.FNativeHandle, @EventID, EventBufferLen,
      EventBuffer, TISC_CALLBACK(@EventCallback), PVoid(Self));
    except ...
...
procedure EventCallback(P: Pointer; Length: Short; Updated: PAnsiChar); cdecl;
begin
  if (Assigned(P) and Assigned(Updated)) then
  begin
    TIBEventThread(P).UpdateResultBuffer(Length, Updated);
    TIBEventThread(P).SignalEvent;
  end;
end;

...

procedure TIBEventThread.UpdateResultBuffer(Length: UShort; Updated: PAnsiChar);
begin
  Move(Updated[0], ResultBuffer[0], Length);
end;

...

procedure TIBEventThread.SignalEvent;
begin
  EventsReceived := True;
  Signal.SetEvent;
end;
      

I see no possibility for the first block to raise an exception! But to do things right the StatusVector must be checked for error codes/messages and a proper exception must be raised. So we come to conclusion that it is Exception object the OnError should be called with. Alternatively the new TZExceptionSpecificData object could be passed as an optional parameter.

And more on this. Currently Pg events are checked synchronously in the main thread. IB uses additional thread but calls event check via Synchronize. I'm not sure what approach I dislike more

[miab3]

Actually I examined IBX code and it seems Zeos' ZIBEventAlerter.pas code is almost 100% copy of IBEvents.pas... o_O I'm really unsure this is OK without even a note in unit header. Probably both versions are inherited from FIB but I don't know the truth.

Apparently everyone(FIB+, IBX Delphi, IBX Lazarus, ZEOS) inherits it from Open Source edition of IBX:
http://wiki.lazarus.freepascal.org/IBX

Michal

[Fr0sT]

miab3, you're right. But I guess the credits must mention original authors, mustn't they?
And maybe we could borrow some code from this fork? They say they've rewritten the unit.

This unit has been almost completely re-written as the original code was
not that robust - and I am not even sure if it worked

[Wild_Pointer]

Fr0sT, marsupilami, miab3
Thank You for your quick and valuable feedback.

There's a;ready ZIBEventAlerter unit with error handling so the change should be done accordingly.
Some code style suggestions:
- const ErrorMessage in TZPgErrorEvent. In 7.3 all ref-counted types in parameters made const or var to avoid excess ref changing.
- FOnError instead of FErrorHandler (why use different name?)
- You do no conversion between PAnsiChar ErrorMsg and String ErrorMessage, with D2009+ there will be warning about implicit conversion; moreover, the DB and native codepages must be considered. Look at ZDbcPostgreSqlUtils.CheckPostgreSQLError function.

Good points. Changes made:
1. const added
2. FErrorHandler renamed to FOnError
3. explicit PAnsiChar to String conversion added. Thank you for pointing the ZDbcPostgreSqlUtils.CheckPostgreSQLError function, I'll look into it. I guess changes must be done to TZPgEventAlerter.HandleNotify too, because no codepage conversions are made there.

Code: Select all

        if Assigned(FOnError) then FOnError(self, E.Message, isHandled);
        if not isHandled then raise;

I did consider raising exceptions if error is not hadled initially, but then realized that this way I would change current behavior of the component and decided playing safe. The other reason - I've found no way (strangely) to handle exceptions from timer events on Lazarus... TApplicationProperties did not catch the exception...
That said maybe isHandled is not needed? Lets do:

Code: Select all

    ...
    try
      CheckPostgreSQLError(...);
    except
      on E: EZSQLException do begin
        if Assigned(FOnError) then FOnError(self, E.Message);
      end;
    end;

?

Regards

[Fr0sT]

I did consider raising exceptions if error is not hadled initially, but then realized that this way I would change current behavior of the component and decided playing safe. The other reason - I've found no way (strangely) to handle exceptions from timer events on Lazarus... TApplicationProperties did not catch the exception...

I doubt that throwing an exception is necessary here. Moreover, I personally dislike the very idea of doing potentially long blocking operations in OnTimer as it will freeze the main thread until the polling is completed. IBX way with background thread seems smarter.
But here an architectural decision is required: what level of changes is enough. Simple patch is easy but some issues remain. The deepest change would be a common interface for background thread and event component, from which any DBC-specific implementation would inherit.

[marsupilami]

This starts to get confusing with all the Interbase and PostgreSQl stuff being mixed.

Actually I examined IBX code and it seems Zeos' ZIBEventAlerter.pas code is almost 100% copy of IBEvents.pas... o_O I'm really unsure this is OK without even a note in unit header. Probably both versions are inherited from FIB but I don't know the truth.

If the the Zeos ZIBEventAlerter.pas really started out as a copy of IBX IBEvents.pas, we might be in trouble because these files were distributed under the InterBase Public License Version 1.0. That't what the original IBEvents.pas says. I doubt that the InterBase Public Licens allows to reuse their code with any other license we wish.
I tried to dig up some information on that. The ZIBEventAlerter.pas started its life as an extra unit supplied by somebody else. It was integrated into Zeos by mdaems after he found it. See the following link: http://zeoslib.sourceforge.net/viewtopi ... 7151#p7151
So it seems that it is not 100% clear on how this unit appered in Zeos. Honesly I like to think that the original author surely was inspired by the original IBEvents.pas but decided to hav his own implementation. There are quite some differences in the component interface and the naming of the procedures.
"Borrowing" code from an IBX fork seems to be a bad way to go - at least until we know if we can copy code that is under the IPL into Zeos.

I did consider raising exceptions if error is not hadled initially, but then realized that this way I would change current behavior of the component and decided playing safe. The other reason - I've found no way (strangely) to handle exceptions from timer events on Lazarus... TApplicationProperties did not catch the exception...
That said maybe isHandled is not needed? Lets do:

Code: Select all

    ...
    try
      CheckPostgreSQLError(...);
    except
      on E: EZSQLException do begin
        if Assigned(FOnError) then FOnError(self, E.Message);
      end;
    end;

Hmmm honestly I don't care if components change behaviour for Zeos 7.3. For Zeos 7.2 it is a different story. Your proposed code just swallows all exceptions that happen - at least all EZSQLExceptions. So if I don't install an error handler everything will look as if it works although it doesn't. For me I have to say, I dislike that. I like programs and components that hit me in the face when something is wrong. This way I at least know that something is wrong. Keeping that in mind my sugestion for this is like this:

• Disable the timer
• Do the work
• Check for errors, throw an exception if there is an error and no error handler
• reenable the timer if there is no error -> For me there is no reason to check for new events if we already had an error.

How did you check the thing about Exceptions with Lazarus? Today I did a very simplistic test with a TTimer that raised an exception. For me it behaved like it should - I got the exception error message. Did I misunderstand you there?

I doubt that throwing an exception is necessary here.

How should we handle errors that are not taken care of? How should the developer know that something went wrong?

Moreover, I personally dislike the very idea of doing potentially long blocking operations in OnTimer as it will freeze the main thread until the polling is completed. IBX way with background thread seems smarter.

Honestly I think that if a check for errors shows some errors a potentially freezing main thread will be your smallest problem. Also raising errors can only be done in the main thread because our components live there. For PostgreSQL I am not sure if older versions of libpq are thread safe. Honestly I don't even know that for the current version. We should check that before we try to implem,ent threading there.

But here an architectural decision is required: what level of changes is enough. Simple patch is easy but some issues remain. The deepest change would be a common interface for background thread and event component, from which any DBC-specific implementation would inherit.

Hmm. I doubt that all libraries we use are thread safe. Even if they are, libraries for future drivers might not be. So if we do something architectural about events I suggest we keep a structure where the driver gets to decide if it wants to use a separate thread for checking for events.

What do you think?

[Wild_Pointer]

Hmmm honestly I don't care if components change behaviour for Zeos 7.3. For Zeos 7.2 it is a different story. Your proposed code just swallows all exceptions that happen - at least all EZSQLExceptions. So if I don't install an error handler everything will look as if it works although it doesn't.

My initial intension was to have as gentle fix as possible in Zeos 7.2. This is the ZeosLib 7.2 Betatest Forum .
The fix just adds a sollution to be able to handle disconnects. It has no effect on legacy code that uses the components. The connection errors did not raise exceptions. The compenent has been introduced years ago, changing its behavior that is backwards incompatible should be avoided in beta test stage. Or am I mistaken?

I like programs and components that hit me in the face when something is wrong. This way I at least know that something is wrong. Keeping that in mind my sugestion for this is like this:

• Disable the timer
• Do the work
• Check for errors, throw an exception if there is an error and no error handler
• reenable the timer if there is no error -> For me there is no reason to check for new events if we already had an error.

You are right. We should not only stop the timer, but set TZPgEventAlerter.Active = false. Calling ZDbcPostgreSqlUtils.CheckPostgreSQLError should also be considered to change the connection state ir connection has been broken.

There are alot of places that should be fixed in the component. Some of them:

• Alerter stays Active when connection is closed (causes AV)
• Does not word in 64bit programs. TZPostgreSQLNotify record should not be packed. Current implementation raises AV in 64bit programs because of allignment problems. (Other structure to record mappings should be revised too.)
• The same component should not be used to manage notifications and to fire events. It causes too much confusion on how to use it. For example the secondary TPgEventAlerter object connected to the main through Processor property doesn't use Interval property. The user must know there should not be used several independent TPgEventAlerter objects connected to single TZConnection... The user of the component must know it from the inside to use it correctly.
• I'm not sure if there should be components in ZeosLib that are related to single DB management system. IMHO there should be one TEventAlerter than has the same properties for all the DBMS. If extra properties are needed they can be provided the same way it is in TZConnection - "Properties".

Because of the last argument I don't know if the component should be fixed in v7.3 or remade into TEventAlerter and its current class removed from the lib.

How did you check the thing about Exceptions with Lazarus? Today I did a very simplistic test with a TTimer that raised an exception. For me it behaved like it should - I got the exception error message. Did I misunderstand you there?

Hmm... Maybe it's lazarus version then.
I've tested this on 1.8.0 Lazarus 64bit. FPC version 3.0.4. The code is quite simple. Attaching...
I've put a breakpoint on ShowMessage(E.Message); and it does not stop there. Message box is not visible either.

For PostgreSQL I am not sure if older versions of libpq are thread safe. Honestly I don't even know that for the current version. We should check that before we try to implem,ent threading there.

The latest PostgrSQL documentation https://www.postgresql.org/docs/10/stat ... ading.html states:

One thread restriction is that no two threads attempt to manipulate the same PGconn object at the same time. In particular, you cannot issue concurrent commands from different threads through the same connection object. (If you need to run concurrent commands, use multiple connections.)

[/quote]

[Fr0sT]

we might be in trouble because these files were distributed under the InterBase Public License Version 1.0. That't what the original IBEvents.pas says. I doubt that the InterBase Public Licens allows to reuse their code with any other license we wish.

I'm not competent in license stuff but borrowing an open source to open source shouldn't cause problems. IPL is based on MPL with some modifications; anyway this question requires checking.

How should we handle errors that are not taken care of? How should the developer know that something went wrong?

IMHO OnError handler should be always assigned. It could even be checked for having a value before doing a request (checked in main thread so the exception could be raised here).

Honestly I think that if a check for errors shows some errors a potentially freezing main thread will be your smallest problem. Also raising errors can only be done in the main thread because our components live there. For PostgreSQL I am not sure if older versions of libpq are thread safe. Honestly I don't even know that for the current version. We should check that before we try to implem,ent threading there.

Hmm. I doubt that all libraries we use are thread safe. Even if they are, libraries for future drivers might not be. So if we do something architectural about events I suggest we keep a structure where the driver gets to decide if it wants to use a separate thread for checking for events.

Well I doubt there are libs SO old to be not thread-safe. At least those libs which implement event mechanism as it must be asynchronous by design. Anyway all this thread/timer stuff must be designed very carefully to handle various tricky situations:
- What if component is within a worker thread that has no message loop? A timer will be handled by main thread potentially freezing an app. And Synchronize() will utilize main thread as well!
- If an event is checked by background thread, OnEvent handler will be called in the context of that thread thus requiring access sync