Re: libpq object hooks (libpq events) - Mailing list pgsql-patches

Andrew Chernow <ac@esilo.com> writes:
> Here is an updated patch for what was called object hooks.  This is now
> called libpq events.  If someone has a better name or hates ours, let us
> know.

This is starting to get there, though I am still desperately unhappy
with the notion of "global" registrations.  As I've said repeatedly,
I don't want that concept in any shape or form: there are no uses for
it that I regard as good ideas.  (And taking out the mutex protection
for the global state isn't making it better, since that makes the
plausible use cases even narrower.)

> Terminology:
> I got rid of calling it object events because it is possible to add
> other non-object-related events in the future; maybe a query event.
> This system can be used for any type of event, I think it is pretty generic.

Hmm.  "Event" for a callback seems a bit weird.  The reasons for calling
the callback are events, and by extension the information structs you
pass to it could legitimately be called events, but the callback isn't
an event.  "Event hook" or "event proc" seem possibly reasonable
nomenclature, though I'd still rather just go with "PGCallback".
Likewise I don't care for "event data", as the data inside that struct
is even less of an event than the proc is.  "Per-callback instance data"
is the best description I can think of but it seems a bit awkward.
I've gone with "instance data" and "passthrough data" in the comments
below.

Some other comments in no particular order:

* I'm inclined to split out the hook-related stuff into a separate
header instead of cluttering libpq-fe.h with it, on the theory that
it has a separate audience; average applications will not be interested
in it.

* The proposed API passes the instance data pointer via the event struct
and the passthrough data pointer as a separate parameter.  This seems
just weird.  Shouldn't we do both the same way?  This ties into my
original thought that the event data struct should be const, on the
grounds that you shouldn't have to initialize it more than once for all
the callbacks.   But I'm not quite sure how you allocate a new instance
data value if that's the case.  Maybe instead of having the ResultCreate
callback scribble on the event data structure, provide an additional
API routine to store the pointer:
    PQresultSetInstanceData(PGresult *res, PGeventProc proc,
                void *instanceData);
This would cost a couple extra cycles compared to what you have,
but surely not much in the normal case where there are very few
hooks registered.  Also, meseems you need such a callback anyway:
what if the hook library desires to realloc its instance data
larger?

* Likewise it's weird to provide PQeventData and PQresultEventData
returning one pointer as function result and one as an output
argument.  I'd suggest four functions returning results only,
perhaps
    void *PQinstanceData(const PGconn *conn, PGEventProc proc)
    void *PQpassthroughData(const PGconn *conn, PGEventProc proc)
    void *PQresultInstanceData(const PGresult *res, PGEventProc proc)
    void *PQresultPassthroughData(const PGresult *res, PGEventProc proc)
The way you have it might save a few cycles in the case where a client
needs both pointers, but I have a feeling that most callers will only
care about one or the other.

            regards, tom lane