Thread: pgsql-server/src/interfaces/libpgtcl pgtclCmds ...

CVSROOT:    /cvsroot
Module name:    pgsql-server
Changes by:    momjian@postgresql.org    02/08/17 08:19:32

Modified files:
    src/interfaces/libpgtcl: pgtclCmds.c pgtclId.c pgtclId.h

Log message:
    What I have done for libpgtcl:
    Everytime if I do PQconsumeInput (when the backend channel gets
    readable) I check for the return value. (0 == error) and generate a
    notification manually, e.g. fixed string connection_closed) and pass it to the
    TCL event queue. The only other thing I had to do is to comment out removing
    all pending events in PgStopNotifyEventSource whenever the connection was
    unexpectedly closed (so the manually generated event will not be deleted).

    A broken backend connection triggers a notify event to the client (fixed
    notification string "connection_closed") so proper action can be taken to switch
    to another database server etc. Remember that this is event driven. If you have
    applications, that have idle database connections most of the time, you'll get
    immediate feedback of a dying server. Upon connection to the server issue a
    pg_notify for notify event "connection_closed" and whenever the backend crashes
    (which it does do in very very rare cases) you get an event driven recovery. (of
    course the Tcl-Event loop has to be processed). Issuing a notification
    "connection_closed" on a still working database could be used for switching to
    another db-server (which I've actually impelemented right now).

    Gerhard Hintermayer

momjian@postgresql.org (Bruce Momjian - CVS) writes:
>     Everytime if I do PQconsumeInput (when the backend channel gets
>     readable) I check for the return value. (0 == error) and generate a
>     notification manually, e.g. fixed string connection_closed) and pass it to the
>     TCL event queue.

Was that patch reviewed by anyone?  I don't think I've even seen it
successfully posted --- the submitter seemed to be having trouble
attaching it.

From the description, I don't think I trust it much.  Event queue stuff
is subtle ...

>   The only other thing I had to do is to comment out removing
>     all pending events in PgStopNotifyEventSource whenever the connection was
>     unexpectedly closed (so the manually generated event will not be deleted).

... and this is *very* likely to break things.

            regards, tom lane

No one reviewed it.  Here is the email exchange I had with the author. I
specifically asked about the downside of the patch.

---------------------------------------------------------------------------

Tom Lane wrote:
> momjian@postgresql.org (Bruce Momjian - CVS) writes:
> >     Everytime if I do PQconsumeInput (when the backend channel gets
> >     readable) I check for the return value. (0 == error) and generate a
> >     notification manually, e.g. fixed string connection_closed) and pass it to the
> >     TCL event queue.
>
> Was that patch reviewed by anyone?  I don't think I've even seen it
> successfully posted --- the submitter seemed to be having trouble
> attaching it.
>
> >From the description, I don't think I trust it much.  Event queue stuff
> is subtle ...
>
> >   The only other thing I had to do is to comment out removing
> >     all pending events in PgStopNotifyEventSource whenever the connection was
> >     unexpectedly closed (so the manually generated event will not be deleted).
>
> ... and this is *very* likely to break things.
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> No one reviewed it.  Here is the email exchange I had with the author.
> [ nothing attached ]

I am beginning to think that patch must have a hex on it.

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > No one reviewed it.  Here is the email exchange I had with the author.
> > [ nothing attached ]
>
> I am beginning to think that patch must have a hex on it.

Oops.  Here it is, in mailbox format.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> I am beginning to think that patch must have a hex on it.

> Oops.  Here it is, in mailbox format.

Having looked it over, I'm not happy about it.  The two big problems are

* hardwired use of "connection_closed" as a NOTIFY condition name.
It might be considered unlikely that this condition name is already in
use out there ... or it might not.

* not removing pending notifies from queue when connection loss
is detected.  This WILL break existing applications (note blithe
reference to possible segfaults in notify scripts in his message).
The reason we are killing those notifies is so that the app won't be
fooled into trying to execute database operations because of receipt
of a stale NOTIFY callback.  While a callback intended specifically
for connection_closed could be expected not to try to do database
operations, I think it's unreasonable to expect existing callbacks for
normal NOTIFY conditions to be coded to guard against this.

I'm also unhappy about the complete lack of documentation.

I'd like to revert this patch and ask Gerhard to try again.

The design I'd suggest is that there be a new command added to libpgtcl
with a format along the lines of
    pg_on_connection_loss dbHandle [ callbackCommand ]
This would be essentially similar to pg_listen except for omitting the
notifyName parameter, and could share a lot of the internal
implementation.  By doing this we could avoid hardwiring an assumption
about an unused notification name.

Also, the code *has* to be rejiggered so that ordinary notify events
are still dropped on connection loss.  And some documentation of this
new command would be appropriate...

            regards, tom lane

OK, patch backed out.  Please address Tom's suggestions and resubmit.
Thanks.


---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >> I am beginning to think that patch must have a hex on it.
>
> > Oops.  Here it is, in mailbox format.
>
> Having looked it over, I'm not happy about it.  The two big problems are
>
> * hardwired use of "connection_closed" as a NOTIFY condition name.
> It might be considered unlikely that this condition name is already in
> use out there ... or it might not.
>
> * not removing pending notifies from queue when connection loss
> is detected.  This WILL break existing applications (note blithe
> reference to possible segfaults in notify scripts in his message).
> The reason we are killing those notifies is so that the app won't be
> fooled into trying to execute database operations because of receipt
> of a stale NOTIFY callback.  While a callback intended specifically
> for connection_closed could be expected not to try to do database
> operations, I think it's unreasonable to expect existing callbacks for
> normal NOTIFY conditions to be coded to guard against this.
>
> I'm also unhappy about the complete lack of documentation.
>
> I'd like to revert this patch and ask Gerhard to try again.
>
> The design I'd suggest is that there be a new command added to libpgtcl
> with a format along the lines of
>     pg_on_connection_loss dbHandle [ callbackCommand ]
> This would be essentially similar to pg_listen except for omitting the
> notifyName parameter, and could share a lot of the internal
> implementation.  By doing this we could avoid hardwiring an assumption
> about an unused notification name.
>
> Also, the code *has* to be rejiggered so that ordinary notify events
> are still dropped on connection loss.  And some documentation of this
> new command would be appropriate...
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073