Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>>
>> It might work to use the address of the hook callback function as
>> a key for retrieving the associated void * pointer. You'd need to
>> not register the same callback function more than once per object,
>> but from what I gather here you don't need to.
>>
>>
>>
>
> Or else have the library return a unique handle when registering hooks,
> rather than supplying a hook name.
>
> cheers
>
> andrew
>
>

The problem with this is that hooks can be registered on a per-conn
basis. Is there a way to ensure the libpq returned handle would be the
unique across every registration of a given PGobjectHooks? ISTM that
the hook handle needs to be constant and unique somehow. Tom's idea
would work with the "very" small limitation of not being able to reuse
hook callbacks. I throw out an idea of using the address of a static,
which is constant and unique.

app_func(PGresult *res)
{
PQresultHookData(res, ?handle?);
}

app_func is not aware of what PGconn generated the result so it has no
idea what libpq returned handle to use.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> It might work to use the address of the hook callback function as
>> a key for retrieving the associated void * pointer. You'd need to
>> not register the same callback function more than once per object,
>> but from what I gather here you don't need to.

> Or else have the library return a unique handle when registering hooks,
> rather than supplying a hook name.

Uh, how would that solve your problem? Seems like the difficulty
shifts from "how do I get the hook data" to "how do I get the key
with which to get the hook data".

regards, tom lane

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Tom Lane wrote:
> Andrew Chernow <ac@esilo.com> writes:
>> There can be cases to use the same callbacks, although unlikely. To
>> completely avoid collisions, the below would work:
>
> Still looks like overdesign to me. If we use the hook function address
> we solve the problem with no extra notation and no extra storage.
>
> Note that if you want N fixed keys, you can just have N hook functions
> (all calling a shared workhorse routine, no doubt). So your proposal
> adds no functionality whatever if the usage involves a fixed number of
> static handles. Now it could possibly allow a variable-at-runtime
> number of handles, but I'd want to see a worked-out use case before
> designing for that much complexity. In particular, it seems to me that
> the problem would then shift to how do you know which handle to use for
> the lookup, thus you've just introduced another layer of complexity
> without buying anything.
>
> I think the typical use case is just that you need to distinguish "your"
> hook from anyone else's hooks, so the function address is plenty
> sufficient.
>
> It should also be noted that this whole problem *can* be solved without
> any PQhookData at all: as long as you have hooks to get control at
> creation and destruction of PGconns and PGresults, you can maintain your
> own index data structure. I'm willing to grant some amount of extra
> API-stuff to save users having to do that in simple cases, but I don't
> think we need to try to support infinitely complex cases.
>
> regards, tom lane
>
>

Okay. No problem over here.

Which callback do we use as the key? Currently, none are required (only
the name was required). We have to choose one callback that must be
provided. Maybe initHookData() must be provided? If the end-user
doesn't need it they can just return NULL.

This is what is passed to PQaddObjectHooks, along with a conn:

typedef struct
{
//char *name; REMOVED

void *data;

/* Invoked when PQsetObjectHook is called. The pointer returned
* by the hook implementation is stored in the private storage of
* the PGconn, accessible via PQhookData(PGconn*). If no
* storage is needed, return NULL or set this hook to NULL.
*/
void *(*initHookData)(const PGconn *conn);

/* Invoked on PQreset and PQresetPoll */
void (*connReset)(const PGconn *conn);

/* Invoked on PQfinish. */
void (*connDestroy)(const PGconn *conn);

/* Invoked on PQgetResult, internally called by all exec funcs */
void *(*resultCreate)(const PGconn *conn, const PGresult *result);

/* Invoked on PQcopyResult */
void *(*resultCopy)(PGresult *dest, const PGresult *src);

/* Invoked when PQclear is called */
void (*resultDestroy)(const PGresult *result);
} PGobjectHooks;

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Andrew Chernow <ac@esilo.com> writes:
> Which callback do we use as the key? Currently, none are required (only
> the name was required). We have to choose one callback that must be
> provided.

What? I thought what you wanted back was the void * pointer that had
been registered with a particular callback function. So you use that
callback function. If it's not actually registered, you get a NULL.

> This is what is passed to PQaddObjectHooks, along with a conn:

This is all wrong IMHO, not least because it creates ABI problems if you
want to add another hook type later. Register each hook separately, eg

typedef void (*PGCRHook) (PGconn *conn, void *passthrough);

void PQregisterConnResetHook(PGconn *conn, PQCRHook func, void *passthrough);

.... repeat for each possible hook ...

regards, tom lane

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Tom Lane wrote:
> Andrew Chernow <ac@esilo.com> writes:
>> Which callback do we use as the key? Currently, none are required (only
>> the name was required). We have to choose one callback that must be
>> provided.
>
> What? I thought what you wanted back was the void * pointer that had
> been registered with a particular callback function. So you use that
> callback function. If it's not actually registered, you get a NULL.
>
>> This is what is passed to PQaddObjectHooks, along with a conn:
>
> This is all wrong IMHO, not least because it creates ABI problems if you
> want to add another hook type later. Register each hook separately, eg
>
> typedef void (*PGCRHook) (PGconn *conn, void *passthrough);
>
> void PQregisterConnResetHook(PGconn *conn, PQCRHook func, void *passthrough);
>
> ... repeat for each possible hook ...
>
> regards, tom lane
>
>

One can make a case to break apart the obj hooks structure into
individual register functions, but I think you have a different idea in
your head than what is being proposed. For starters, there is no
passthru pointer to register with a callback (there could be but that is
different than hook data...your register looks more like a user_ptr).
The passthru pointer, what we call hookData, is allocated with a PGconn
(not provided by the user). This is the point of the initHookData callback.

typedef void *(*PGinitHookData)(const PGconn *conn);

PQregisterInitHookData((PGconn *)NULL, (PGinitHookData)func);
PQregisterConnResetHook((PGconn *)NULL, (PGCRHook)func);
//etc...
conn = PQconnectdb();

When connectdb returns, initHookData has already been called. So, a
call to PQhookData(conn, ????) will work. BUT, what is still missing
from the equation is how to uniquely reference hookData on a conn.

What I was previously suggesting was to use the address of initHookData,
since w/o this address there wouldn't be any hook data to get. Seemed
like a logical choice.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Tom Lane wrote:
> Andrew Chernow <ac@esilo.com> writes:
>> Which callback do we use as the key? Currently, none are required (only
>> the name was required). We have to choose one callback that must be
>> provided.
>
> What? I thought what you wanted back was the void * pointer that had
> been registered with a particular callback function. So you use that
> callback function. If it's not actually registered, you get a NULL.
>
>> This is what is passed to PQaddObjectHooks, along with a conn:
>
> This is all wrong IMHO, not least because it creates ABI problems if you
> want to add another hook type later. Register each hook separately, eg
>
> typedef void (*PGCRHook) (PGconn *conn, void *passthrough);
>
> void PQregisterConnResetHook(PGconn *conn, PQCRHook func, void *passthrough);
>
> ... repeat for each possible hook ...
>
> regards, tom lane
>
>

I am starting to think we have not clarified what it is we are trying to
do; maybe hook is the wrong terminology.

We need to add members to a conn and result, that's pretty much it. To
do this, an api user can register callbacks to receive notifications
about created/destroyed states of objects. PQhookData is just like
PQerrorMessage in that both are public accessor functions to private
object data. The difference is that there can be more than one hookData
"dynamic struct member" on a conn/result at a time, unlike errorMessage;
thus the need for an additional "lookup" value when getting hook data
(what was hookName).

A solution is to only have one function with an eventId, instead of a
register function per event. I am playing with using the name 'event'
rather than hook.

typedef enum
{
PQEVTID_INITDATA,
PQEVTID_CONNRESET,
// resultcreate, resultcopy, etc...
} PGobjectEventId;

typedef void *(*PGobjectEventProc)(PGobjectEventId evtId, ...);

int PQregisterObjectEventProc(PGconn*, PGobjectEventProc);

// no more key (hookName), use PGobjectEventProc address
void *PQeventData(PGconn *, PGobjectEventProc);
void *PQresultEventData(PGresult *, PGobjectEventProc);

Now there is just one libpq register function and an enum; very
resilient to future additions and ABI friendly. The API user will
follow a convention of: if you don't care about an event or don't know
what it is, just return NULL from the eventProc function (ignore it).

The implementation of a PGobjectEventProc would most likely be a switch
on the PGobjectEventId with a couple va_arg() calls (which would be very
well documented).

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

On Thu, May 15, 2008 at 8:38 PM, Andrew Chernow <ac@esilo.com> wrote:
> We need to add members to a conn and result, that's pretty much it. To do
> this, an api user can register callbacks to receive notifications about
> created/destroyed states of objects. PQhookData is just like PQerrorMessage
> in that both are public accessor functions to private object data. The
> difference is that there can be more than one hookData "dynamic struct
> member" on a conn/result at a time, unlike errorMessage; thus the need for
> an additional "lookup" value when getting hook data (what was hookName).

> typedef void *(*PGobjectEventProc)(PGobjectEventId evtId, ...);
> int PQregisterObjectEventProc(PGconn*, PGobjectEventProc);
> void *PQeventData(PGconn *, PGobjectEventProc);
> void *PQresultEventData(PGresult *, PGobjectEventProc);

This provides what we need...a key to lookup the hook data without
using a string. Also, it reduces the number of exports (it's a little
easier for us, while not essential, to not have to register each
callback individually). Also, this AFAICT does not have any ABI
issues (no struct), and adds less exports which is nice. We don't
have to 'look up' the data inside the callbacks..it's properly passed
through as an argument. While vararg callbacks may be considered
unsafe in some scenarios, I think it's a good fit here.

The most important part though is that it fits what we think is needed
to maintain the data we associate with the libpq with the proper
lifetime. I'm not sure that everyone was on the same page in terms of
how this works...we may not have explained ourselves properly. In our
defense, the interaction between libpq and a wrapping library like
libpqtypes is a bit involved and the 'best possible' way to link
things up did not necessarily suggest itself the first time out.

We would like to wrap this up into some form the community would
accept. The event proc style of doing this is better than our initial
approach...faster and cleaner. In fact, we are pleased with all the
changes that have come about due to community suggestions...there are
many positive results.

merlin

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

"Merlin Moncure" <mmoncure@gmail.com> writes:
>> typedef void *(*PGobjectEventProc)(PGobjectEventId evtId, ...);
>> int PQregisterObjectEventProc(PGconn*, PGobjectEventProc);
>> void *PQeventData(PGconn *, PGobjectEventProc);
>> void *PQresultEventData(PGresult *, PGobjectEventProc);

> This provides what we need...a key to lookup the hook data without
> using a string. Also, it reduces the number of exports (it's a little
> easier for us, while not essential, to not have to register each
> callback individually). Also, this AFAICT does not have any ABI
> issues (no struct), and adds less exports which is nice. We don't
> have to 'look up' the data inside the callbacks..it's properly passed
> through as an argument. While vararg callbacks may be considered
> unsafe in some scenarios, I think it's a good fit here.

I don't think varargs callbacks are a good idea at all. Please adjust
this to something that doesn't do that. Also, haven't you forgotten
the passthrough void *?

If you really want only one callback function, perhaps what would work
is

typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo,
void *passthrough);

int PQregisterEventProc(PGconn *conn, PGeventProc proc, void *passthrough);

where eventInfo will point to one of several exported struct types
depending on the value of eventId. With this approach, you can add
more fields to the end of any one such struct type without creating
ABI issues. I have little confidence in being able to make similar
changes in a varargs environment.

Also, even if varargs are safe they'd be notationally unpleasant
in the extreme. varargs are just a PITA to work with --- you'd have
to do all the decoding in the first-level hook routine, even for
items you weren't going to use. With something like the above
all you need is a switch() and some pointer casts.

regards, tom lane

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

On Fri, May 16, 2008 at 10:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Merlin Moncure" <mmoncure@gmail.com> writes:
>>> typedef void *(*PGobjectEventProc)(PGobjectEventId evtId, ...);
>>> int PQregisterObjectEventProc(PGconn*, PGobjectEventProc);
>>> void *PQeventData(PGconn *, PGobjectEventProc);
>>> void *PQresultEventData(PGresult *, PGobjectEventProc);
>
>> This provides what we need...a key to lookup the hook data without
>> using a string. Also, it reduces the number of exports (it's a little
>> easier for us, while not essential, to not have to register each
>> callback individually). Also, this AFAICT does not have any ABI
>> issues (no struct), and adds less exports which is nice. We don't
>> have to 'look up' the data inside the callbacks..it's properly passed
>> through as an argument. While vararg callbacks may be considered
>> unsafe in some scenarios, I think it's a good fit here.
>
> I don't think varargs callbacks are a good idea at all. Please adjust
> this to something that doesn't do that. Also, haven't you forgotten
> the passthrough void *?

We didn't...one of the functions (the init) doesn't need it so we
didn't expose it to the fixed arguments...we would just va_arg it off
in the other cases. Regardless, your comments below make that moot:

> If you really want only one callback function, perhaps what would work
> is
>
> typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo,
> void *passthrough);
>
> int PQregisterEventProc(PGconn *conn, PGeventProc proc, void *passthrough);
>
> where eventInfo will point to one of several exported struct types
> depending on the value of eventId. With this approach, you can add
> more fields to the end of any one such struct type without creating
> ABI issues. I have little confidence in being able to make similar
> changes in a varargs environment.

this is fine.

> Also, even if varargs are safe they'd be notationally unpleasant
> in the extreme. varargs are just a PITA to work with --- you'd have
> to do all the decoding in the first-level hook routine, even for
> items you weren't going to use. With something like the above
> all you need is a switch() and some pointer casts.

Switch, plus struct (basically a union) will do the trick nicely. Can
it be a formal union, or is it better as a void*?

The main issue was how what we called the 'hook data' was passed back
and forth. We'll get a patch up.

merlin

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Merlin Moncure wrote:
>
>> Also, even if varargs are safe they'd be notationally unpleasant
>> in the extreme. varargs are just a PITA to work with --- you'd have
>> to do all the decoding in the first-level hook routine, even for
>> items you weren't going to use. With something like the above
>> all you need is a switch() and some pointer casts.
>>
>
> Switch, plus struct (basically a union) will do the trick nicely. Can
> it be a formal union, or is it better as a void*?
>
> The main issue was how what we called the 'hook data' was passed back
> and forth. We'll get a patch up.
>
>
>

All of this is getting quite a long way from what was in the commitfest
queue. Do we still want to try to get this in this cycle, or should it
be marked returned to author for more work?

cheers

andrew

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches