Andrew Dunstan <andrew@dunslane.net> writes:
> It should be noted that while this feels slightly foreign, it isn't
> hugely invasive, unlike the previous effort - it's only a few hundred
> lines of new code.

> If we reject this, presumably the authors will have no alternative than
> to offer libpqtypes as a patch to libpq.

No, they could revise their patch to be more stylistically in keeping
with libpq. I haven't looked at the current version of the patch yet,
but the early versions seemed quite overengineered to me, so your
criticism didn't surprise me.

>> Keep in mind that the original patch supported a single hook being
>> registered.

> Right, it was more the case insensitive part that bothered me.

I'm wondering why the hooks need names at all. AFAICS all that
libpq needs to know about a hook is a callback function address
and a void * passthrough pointer.

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 Wed, May 14, 2008 at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm wondering why the hooks need names at all. AFAICS all that
> libpq needs to know about a hook is a callback function address
> and a void * passthrough pointer.

For reference...here is what libpqtypes has to do to bind via the
hooking interface:
http://libpqtypes.esilo.com/browse_s...l?file=hooks.c

Are you proposing something substantially different (not my handle
suggestion)? How would it work exactly?

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:
> On Wed, May 14, 2008 at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm wondering why the hooks need names at all. AFAICS all that
>> libpq needs to know about a hook is a callback function address
>> and a void * passthrough pointer.
>
> For reference...here is what libpqtypes has to do to bind via the
> hooking interface:
> http://libpqtypes.esilo.com/browse_s...l?file=hooks.c
>
> Are you proposing something substantially different (not my handle
> suggestion)? How would it work exactly?
>
> merlin
>
>

It is important to see how "NON-hook-callback" functions in libpqtypes
make use of the hook data.

PQparamCreate must get a pointer to the conn hook data
http://libpqtypes.esilo.com/browse_s...param.c#line24

PQgetf must get a pointer to the result hook data
http://libpqtypes.esilo.com/browse_s...=exec.c#line65

These are NOT hook callbacks. The hook data is NOT isolated to callback
functions. It is memory that is publically accessible, outside hook
implementations.

--
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 Wed, May 14, 2008 at 10:52:43AM -0400, Bruce Momjian wrote:
>
> One idea would be to add the libpq hooks but not document them. This
> way, we can modify or remove the API as needed in the future. As
> libpqtypes matures and we are sure what the API should be, we can
> document it as stable and permanent.

-1

Perhaps it is just me, but undocumented interface are evil. Simply document
it with the changable bits labled as such.

-dg

--
David Gould daveg@sonic.net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.

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

On Wed, May 14, 2008 at 3:47 PM, daveg <daveg@sonic.net> wrote:
> On Wed, May 14, 2008 at 10:52:43AM -0400, Bruce Momjian wrote:
>>
>> One idea would be to add the libpq hooks but not document them. This
>> way, we can modify or remove the API as needed in the future. As
>> libpqtypes matures and we are sure what the API should be, we can
>> document it as stable and permanent.
>
> Perhaps it is just me, but undocumented interface are evil. Simply document
> it with the changable bits labled as such.

Well, in defense of Bruce, there is some precedent for that. Anything
that queries binary currently falls under that umbrella (mostly
undocumented and changeable). For functions exported by libpq
though...it's probably better to nail things down as much as possible
up front.

This is a big advantage of the hooks strategy overall, it allows us to
mature the library except for the interaction with libpq (ISTM Bruce
was trying to give us a little leeway here as well, and we appreciate
that).

merlin

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

On Wed, May 14, 2008 at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm wondering why the hooks need names at all. AFAICS all that
> libpq needs to know about a hook is a callback function address
> and a void * passthrough pointer.

I'm trying to make this work as you suggest.

It's pretty clear that all the callbacks should be modified to send a
void* along with the other arguments. This would eliminate the need
for hookname there (getting the state inside callback functions).

The problem is the functions PQhookData(conn, hookname) and
PQresultHookData(result, hookName). We need these to work in
functions that are not callbacks. If we eliminate hookname
completely, there is no way for libpq to know which private state we
are asking for. I'm a little bit stuck here. Is it possible to
preserve the hookname outside of the context of the callback functions
or is there something else I'm missing? (I'm sorry if I'm being
obtuse)

Eliminating the need for libpqtypes to need PQhookx functions, while
possible, would make libpqtypes a lot more difficult to use and
generally more awkward.

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:
> The problem is the functions PQhookData(conn, hookname) and
> PQresultHookData(result, hookName). We need these to work in
> functions that are not callbacks. If we eliminate hookname
> completely, there is no way for libpq to know which private state we
> are asking for.

Well, depending on a hook name for this is broken-by-design anyway,
because there is no way for two independently written libraries to
be sure they don't choose conflicting hook names. So the need for
a hook name has to go away.

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.

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:
> "Merlin Moncure" <mmoncure@gmail.com> writes:
>
>> The problem is the functions PQhookData(conn, hookname) and
>> PQresultHookData(result, hookName). We need these to work in
>> functions that are not callbacks. If we eliminate hookname
>> completely, there is no way for libpq to know which private state we
>> are asking for.
>>
>
> Well, depending on a hook name for this is broken-by-design anyway,
> because there is no way for two independently written libraries to
> be sure they don't choose conflicting hook names. So the need for
> a hook name has to go away.
>
> 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

--
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:
> "Merlin Moncure" <mmoncure@gmail.com> writes:
>> The problem is the functions PQhookData(conn, hookname) and
>> PQresultHookData(result, hookName). We need these to work in
>> functions that are not callbacks. If we eliminate hookname
>> completely, there is no way for libpq to know which private state we
>> are asking for.
>
> Well, depending on a hook name for this is broken-by-design anyway,
> because there is no way for two independently written libraries to
> be sure they don't choose conflicting hook names. So the need for
> a hook name has to go away.
>
> 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.
>
> regards, tom lane
>
>

There can be cases to use the same callbacks, although unlikely. To
completely avoid collisions, the below would work:

Use the address of a static, maybe an 'int', as a hook hanlde. Provide
the user with a macro that can make a hook handle.

typedef void *PGhookHandle;

// Declare an int and point "tokname" at it. The value doesn't
// matter, its the pointer address we are interested in.
#define PQ_MAKE_HOOK_HANDLE(tokname) \
static int hh__ ## tokname = 0; \
static const PGhookHandle tokname = &hh__ ## tokname

As an example, here is what libpqtypes would do:

// libpqtypes hooks.c
PQ_MAKE_HOOK_HANDLE(pqthookhandle);

Now the handle replaces the hookName. The "const char *hookName" member
of the PQobjectHooks structure is changed to "const PGhookHanlde
hookHandle". This allows for all the flexibility of a const char * w/o
the collision issues.

// these function prototypes change as well
void *PQhookData(PGconn *, const PGhookHandle);
void *PQresultHookData(PGresult *, const PGhookHandle);

We will send in an updated patch.

--
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 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