libpq object hooks

Merlin Moncure wrote:

Attached is an updated version of 'libpq object hooks'. The primary
purpose for libpq object hooks is to support our libpqtypes system
(not attached), but could possibly some nice sideband features to
libpq. We are hoping to sneak this into the May commit fest.

I've had a preliminary look at this.

The first thing it needs is lots and lots of documentation. I think it
probably needs a Section in the libpq chapter all on its own, preferably
with some examples. I think that lack alone is enough to keep it from
being committed for now.

Second, the hook names are compared case insensitively and by linear
search. I don't see any justification for using case insensitive names
for hooks in a C program, so I think that part should go. And if we
expect to keep anything other than trivial numbers of hooks we should
look at some sort of binary or hashed search.

The thing that is a bit disturbing is that the whole style of this
scheme is very different from the fairly simple APIs that the rest of
libpq presents. It's going to make libpq look rather odd, I think. I
would have felt happier if the authors had been able to come up with a
simple scheme to add API calls to export whatever information they
needed, rather than using this callback scheme.

That said, this patch doesn't look that bad to me otherwise, at least on
first inspection. One might say the the ability to add tuples to a
resultset arbitrarily, or to change an attribute arbitrarily, might be
footguns (and if you can add one, why can't you delete one?), but then
this is data in the hands of the client anyway, so they can do what they
like with it after they get it out of the resultset, so I guess there's
no real danger.

cheers

andrew

Andrew Dunstan wrote:

The first thing it needs is lots and lots of documentation. I think it
probably needs a Section in the libpq chapter all on its own, preferably
with some examples. I think that lack alone is enough to keep it from
being committed for now.

Second, the hook names are compared case insensitively and by linear
search. I don't see any justification for using case insensitive names
for hooks in a C program, so I think that part should go. And if we
expect to keep anything other than trivial numbers of hooks we should
look at some sort of binary or hashed search.

The thing that is a bit disturbing is that the whole style of this
scheme is very different from the fairly simple APIs that the rest of
libpq presents. It's going to make libpq look rather odd, I think. I
would have felt happier if the authors had been able to come up with a
simple scheme to add API calls to export whatever information they
needed, rather than using this callback scheme.

My personal opinion is still that I would like to see a more general
usefulness for these functions before adding them to libpq. The
complexity of the API just mirrors my gut feeling on this.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Dunstan escribi�:

The thing that is a bit disturbing is that the whole style of this
scheme is very different from the fairly simple APIs that the rest of
libpq presents. It's going to make libpq look rather odd, I think. I
would have felt happier if the authors had been able to come up with a
simple scheme to add API calls to export whatever information they
needed, rather than using this callback scheme.

I'm not sure I understand this point. Remember that this is here to
support the libpqtypes library. There doesn't seem to be a way for an
API such as you describe to work.

Second, the hook names are compared case insensitively and by linear
search. I don't see any justification for using case insensitive names
for hooks in a C program, so I think that part should go. And if we
expect to keep anything other than trivial numbers of hooks we should
look at some sort of binary or hashed search.

Keep in mind that the original patch supported a single hook being
registered. Perhaps we could dream about having a couple of hooks
registered, but turning into hashed search would seem to be overkill, at
least for now. (If hooking into libpq is truly successful we can always
improve it later -- it's not an exported detail of the API after all.)

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:

Andrew Dunstan escribi�:

The thing that is a bit disturbing is that the whole style of this
scheme is very different from the fairly simple APIs that the rest of
libpq presents. It's going to make libpq look rather odd, I think. I
would have felt happier if the authors had been able to come up with a
simple scheme to add API calls to export whatever information they
needed, rather than using this callback scheme.

I'm not sure I understand this point. Remember that this is here to
support the libpqtypes library. There doesn't seem to be a way for an
API such as you describe to work.

That might well be true. The issue then becomes "Do we want to add
something with this flavor to libpq?" I take it Bruce's answer is "No",
at least until he has seen more evidence of general usefulness. I think
we need to make a decision on this before anyone wastes any more time.

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. ISTM that we're then asking
them to climb over a fairly high hurdle. On the one hand we want them to
demonstrate that there's demand for their tool and on the other we make
it difficult to distribute and deploy.

Second, the hook names are compared case insensitively and by linear
search. I don't see any justification for using case insensitive names
for hooks in a C program, so I think that part should go. And if we
expect to keep anything other than trivial numbers of hooks we should
look at some sort of binary or hashed search.

Keep in mind that the original patch supported a single hook being
registered. Perhaps we could dream about having a couple of hooks
registered, but turning into hashed search would seem to be overkill, at
least for now. (If hooking into libpq is truly successful we can always
improve it later -- it's not an exported detail of the API after all.)

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

cheers

andrew

On Wed, May 14, 2008 at 8:18 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

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

Done. We in fact had realized this was a mistake anyways following
some profiling of the libpqtypes library. In some scenarios, this
function gets called a lot.

Regarding the other comments:
*) API structure: Our major objective was to minimize exports to
libpq. I think both copyresult and setvalue have some possible
sideband usage (footguns or no). Additional functions could be
speculated but are not required by libpqtypes. We would have no
problem adding a few things to complete the api if necessary.

The patch is basically the minimum libpqtypes needs and has to work
more or less as written. We tried a few times to suggest implementing
the split a different way (basically, more invasion into libpq). We
couldn't get any action there.

If the patch is rejected on general merits...that signals the death
blow for libpqtypes. We have a chicken/egg problem...people can't use
it without patching libpq which will really hamper its adoption, which
is, uh, needed to justify the patch. For the record, we have had a
couple of dozen downloads of the libpqtypes library on pgfoundry since
we put it up there last week. Based on how it has simplified and
improved our own code vs. libpq, we have absolutely no doubts it is a
major improvement over PQexecParams.

merlin

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

On Tue, May 13, 2008 at 11:52 PM, Bruce Momjian <bruce@momjian.us> wrote:

My personal opinion is still that I would like to see a more general
usefulness for these functions before adding them to libpq. The
complexity of the API just mirrors my gut feeling on this.

There has been a lot of demand for the features that libpqtypes
provides...a quick search of the archives demonstrates this. Here are
a few examples of threads from users asking or even trying to
implement some of the things that libpqtypes helps with or indirectly
solves...I am speaking to your point of usefulness here.

[BUGS] BUG #4053: libpq documentation should express clearly, that
integers are passed in network octet order
[PATCHES] [PATCH] automatic integer conversion
[HACKERS] convert int to bytea
[HACKERS] comunication protocol
[HACKERS] PQescapeBytea* version for parameters
[HACKERS] libpq and Binary Data Formats
[GENERAL] binary representation of date and numeric
[GENERAL] binding 64-bit integer
[HACKERS] Last minute mini-proposal (I know, I know) for PQexecf()
[PERFORM] Low throughput of binary inserts from windows to linux
[GENERAL] Storing images as BYTEA or large objects

merlin

Merlin Moncure wrote:

Regarding the other comments:
*) API structure: Our major objective was to minimize exports to
libpq. I think both copyresult and setvalue have some possible
sideband usage (footguns or no). Additional functions could be
speculated but are not required by libpqtypes. We would have no
problem adding a few things to complete the api if necessary.

The patch is basically the minimum libpqtypes needs and has to work
more or less as written. We tried a few times to suggest implementing
the split a different way (basically, more invasion into libpq). We
couldn't get any action there.

If the patch is rejected on general merits...that signals the death
blow for libpqtypes. We have a chicken/egg problem...people can't use
it without patching libpq which will really hamper its adoption, which
is, uh, needed to justify the patch. For the record, we have had a
couple of dozen downloads of the libpqtypes library on pgfoundry since
we put it up there last week. Based on how it has simplified and
improved our own code vs. libpq, we have absolutely no doubts it is a
major improvement over PQexecParams.

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.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Wed, May 14, 2008 at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

I think you'll find the latest version more reasonable. We tried to
keep the over-engineering, so to speak, on our side and make the libpq
changes surgical.

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.

Here are the proposed API changes [aside: this is one of two breaks
from your initial suggestions..the other being PQcopyResult]:

+ PQcopyResult              142
+ PQsetvalue                143
+ PQresultAlloc             144
+ PQaddObjectHooks          145
+ PQaddGlobalObjectHooks    146
+ PQhookData                147
+ PQresultHookData          148

In question is:
+ void *
+ PQhookData(const PGconn *conn, const char *hookName)

Basically, libpqtypes has various functions that take a PGconn that
need the private data that is stored in libpq with the connection.
PQhookData just does simple linear search and returns the data.

[thinks]
are you suggesting something like
+ void *
+ PQhookData(const PGconn *conn, const void *hookHandle)
?

I would have to take a quick look at the code with Andrew C (he'll be
in in a bit)...but this might be doable.

merlin

On Wed, May 14, 2008 at 10:52 AM, Bruce Momjian <bruce@momjian.us> wrote:

Merlin Moncure wrote:

Regarding the other comments:
*) API structure: Our major objective was to minimize exports to
libpq. I think both copyresult and setvalue have some possible
sideband usage (footguns or no). Additional functions could be
speculated but are not required by libpqtypes. We would have no
problem adding a few things to complete the api if necessary.

The patch is basically the minimum libpqtypes needs and has to work
more or less as written. We tried a few times to suggest implementing
the split a different way (basically, more invasion into libpq). We
couldn't get any action there.

If the patch is rejected on general merits...that signals the death
blow for libpqtypes. We have a chicken/egg problem...people can't use
it without patching libpq which will really hamper its adoption, which
is, uh, needed to justify the patch. For the record, we have had a
couple of dozen downloads of the libpqtypes library on pgfoundry since
we put it up there last week. Based on how it has simplified and
improved our own code vs. libpq, we have absolutely no doubts it is a
major improvement over PQexecParams.

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.

The API functions relating to hooks are unlikely to change once
settled on...but PQsetvalue/PQcopyResult are a good fit with your idea
(they introduce new behaviors that could possibly be used outside of
libpqtypes context, and could require changes down the line).

merlin

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.

In question is:
+ void *
+ PQhookData(const PGconn *conn, const char *hookName)

Basically, libpqtypes has various functions that take a PGconn that
need the private data that is stored in libpq with the connection.
PQhookData just does simple linear search and returns the data.

[thinks]
are you suggesting something like
+ void *
+ PQhookData(const PGconn *conn, const void *hookHandle)
?

I would have to take a quick look at the code with Andrew C (he'll be
in in a bit)...but this might be doable.

The hook callback functions allow the hook implementor to receive
created/destroyed events about a PGconn and PGresult (PQreset as well). The
hook implementor has the option of associating some memory with either. But,
that memory pointer is worthless unless there is a way of referencing it at a
later time.

HookName would not be needed if the libpq hook API only supported a single
Object Hook to be registered per conn (or library-wide). It was requested of us
to allow a list of hooks per conn. This requries a way of referencing the item.

Functions outside the hook callback functions:
- PQparamCreate needs libpq-hook-func void *PQhookData(conn, hookName)
- PQgetf needs libpq-hook-func void *PQresultHookData(res, hookName)
- Also, the "void *resultCreate(...)" hook callback implementation inside
libpqtypes must use PQhookData on the provided conn so it can copy some
conn.hookData properties to the result.hookData. The created result.hookData is
returned (one can return NULL if no hookData is needed).

I have no issue with merlin's idea of a void *handle, but that doesn't really
change the concept at all ... just allows someone registering hooks with libpq
to use something other than a string. The hookName string idea feels a little
more natural and simple.

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

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_source.html?file=hooks.c

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

merlin

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_source.html?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_source.html?file=param.c#line24

PQgetf must get a pointer to the result hook data
http://libpqtypes.esilo.com/browse_source.html?file=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/

Attached is an updated patch. The only change to this patch is that
hookNames are now compared with strcmp rather than strcasecmp. The
comparisons occur in fe-mics.c (bottom of file) during PQhookData and
PQresultHookData.

Not sure this needs clarification, but PQcopyResult, PQresultAlloc and
PQsetvalue are not part of the object hooks API. They are part of
libpq's result management functions (at least that is what I call them).

Hook API
- PQaddObjectHooks
- PQaddGlobalObjectHooks ** CAN BE REMOVED, SEE BELOW
- PQhookData
- PQresultHookData

Result Management (I would put PQmakeEmptyResult here)
- PQcopyResult
- PQsetvalue
- PQresultAlloc (light wrapper to internal pqResultAlloc)

PROPOSAL:
PQaddGlobalObjectHooks can be removed by handling per-conn and global
hook registeration through PQaddObjectHooks. If the provided PGconn is
NULL, add hooks to global libpq list:

int
PQaddObjectHooks(PGconn *conn, PGobjectHooks *hooks)
{
if(conn == NULL)
;// global hook register
else
;// per-conn register
}

This would make the Object Hooks API 3 functions instead of 4. The same
rules apply to global hook registeration, do this from main() before
using libpq as the ObjectHooks list is not thread-safe (no locking).

NOTE: The patch is called objhooks.patch which is a misleading. The
patch is really comprised of the API calls needed to make libpqtypes
work. If anyone feels this should be broken into two patches (like
objhooks.patch and resmgnt.patch), let us know.

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

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.

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

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

"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

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