Re: libpq object hooks (libpq events)

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.

regards,
merlin

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(at)momjian(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)sonic(dot)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(at)sonic(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)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(at)gmail(dot)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

Tom Lane wrote:
> "Merlin Moncure" <mmoncure(at)gmail(dot)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/

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/

Andrew Dunstan <andrew(at)dunslane(dot)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

Andrew Chernow <ac(at)esilo(dot)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

Tom Lane wrote:
> Andrew Chernow <ac(at)esilo(dot)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/

Andrew Chernow <ac(at)esilo(dot)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

Tom Lane wrote:
> Andrew Chernow <ac(at)esilo(dot)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/

Tom Lane wrote:
> Andrew Chernow <ac(at)esilo(dot)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/

On Thu, May 15, 2008 at 8:38 PM, Andrew Chernow <ac(at)esilo(dot)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

"Merlin Moncure" <mmoncure(at)gmail(dot)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

On Fri, May 16, 2008 at 10:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Merlin Moncure" <mmoncure(at)gmail(dot)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

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

"Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
> Switch, plus struct (basically a union) will do the trick nicely. Can
> it be a formal union, or is it better as a void*?

I don't think a union buys much notational convenience or safety here,
although admittedly it's a close question. In one case you're trusting
to cast the pointer to the appropriate type, in the other you're
trusting to use the right union member. One advantage of separate
structs is that there's no reason not to make the struct type names
long enough to be clear, whereas there's a very strong notational
temptation to make union member names short, because you'll be typing
them a lot.

regards, tom lane

On Fri, May 16, 2008 at 11:21 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> 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?

That's your call...we can have a patch up within 24 hours or so. My
suggestion would be to let us put up an updated version in the May
queue, and if there are any further serious objections we can push it
to the next queue giving some more time for things to percolate.

merlin

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> 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?

So far I think it still falls within the category of allowing the author
to revise his work. I don't want to hold commitfest open waiting on
revisions of this patch, but as long as there's still other stuff being
worked through I don't see why they can't keep trying.

Just for the record, I would really like to close this fest before
PGCon starts. We still have a couple more days to get it done.

regards, tom lane

Andrew Dunstan escribió:

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

There are still patches open for which no discussion has even started,
so I think this is OK for this commitfest.

(Besides, it seems we're almost there on this patch.)

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

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> 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?
>>
>
> So far I think it still falls within the category of allowing the author
> to revise his work. I don't want to hold commitfest open waiting on
> revisions of this patch, but as long as there's still other stuff being
> worked through I don't see why they can't keep trying.
>
> Just for the record, I would really like to close this fest before
> PGCon starts. We still have a couple more days to get it done.
>
>
>

Apart from this one only two have not been claimed:

. Map Forks
. TRUNCATE TABLE with IDENTITY

cheers

andrew

Tom Lane wrote:
>
> typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo,
> void *passthrough);
>
> int PQregisterEventProc(PGconn *conn, PGeventProc proc, void *passthrough);
>
>
>

The above prototypes will work and we will add our 'event instance
pointer' to the event info structures. Should have a patch shortly.

libpqtypes doesn't need a passthrough/user-pointer. The object
events/hooks allocate memory when the object is created "part of a
conn/result object instance", it is not supplied by the API user
registering the event/hook callback.

I think this is where some confusion has been occurring, there are two
different pointers: user pointer and event instance pointer.

BTW, PQeventData and PQresultEventData return the event instance
pointer, not the passthrough. At least that is how we were using these
functions, being how our previous patches do not include a
passthrough/user-pointer feature because libpqtypes didn't need it.

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

On Fri, May 16, 2008 at 2:34 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Tom Lane wrote:
>>
>> typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo,
>> void *passthrough);
>>
>> int PQregisterEventProc(PGconn *conn, PGeventProc proc, void
>> *passthrough);

> The above prototypes will work and we will add our 'event instance pointer'
> to the event info structures. Should have a patch shortly.

Right. I actually overlooked the 'passthrough' in
PQregisterEventProc. It turns out that we are still not quite on the
same page and this needs to be clarified before we move on. The
passthrough cannot exist...the correct prototypes (reasoning will
follow) are:

typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo);
int PQregisterEventProc(PGconn *conn, PGeventProc proc);
PQhookData(const PGconn* conn, PGeventProc proc);
...etc.

The purpose of the callbacks is to allow a hooking library to
establish private data that is associated with a PGconn or a PGresult.
Invoking PQregisterEventProc tells libpq 'I am interested in doing
this', for the supplied PGconn, future results created with that
connection, and (if PGconn is passed as null), all future connections.

Once that is established, libpq begins telling the hooking library
when and what needs to be allocated or deleted. This is done for both
connections and results at the appropriate times and the management
always occurs inside a callback function. Thus, while the hooking
library controls what is in the state data, _it does not control when
it is created or destroyed_. Passing a void* into PQregisterEventProc
suggests that is the wrapper's purvue to establish the private
information. It isn't, and it can't be..it's as simple as that.

For example, consider that 'hooked' PGconn then produces a result. If
a passthrough as Tom suggested were passed around, what would you do
to it when the connection was freed (which throws a connection closing
event)? What exactly is passthrough pointing to in a result copy
event? If passed around as a single poitner, they are not
defined.These are pointers to different structures who are created at
particular times that only libpq knows.

All of the callbacks (except for possibly conn reset) are basically
allocation and deletion events. For example, result create is libpq
telling the hooking library: 'a result is coming, please initialize
your result extensions now'. At that point, a very particular
structure is passed back to the hooking library:

{
const PGconn* conn; // the source conn the result
const void *conn_state; // the conn's private state (it might be
interesting, and we don't want to look it up later)
const PGresult* res; // the new result's poiter
void *out_res_state; // we are going to allocate something and set
this in the callback
}

As you can see, there is more going on here than a single passthrough
pointer can handle. We are in fact doing what a passthrough provides
in principle. I can certainly understand why our original 'lookup by
name' concept threw up a smokescreen (it did obfuscate the passing
requirements), but hooking can't be serviced by a single pointer set
at callback registration.

With the above prototypes and carefully designed structures,
everything feels natural. libpq is defining exactly what is supposed
to happen, and the structure parameters define what is coming in and
out. Things are being 'passed through'...but in a more discrete way.

merlin

On Fri, May 16, 2008 at 3:49 PM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
> On Fri, May 16, 2008 at 2:34 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> Tom Lane wrote:
>>>
>>> typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo,
>>> void *passthrough);
>>>
>>> int PQregisterEventProc(PGconn *conn, PGeventProc proc, void
>>> *passthrough);
>
>> The above prototypes will work and we will add our 'event instance pointer'
>> to the event info structures. Should have a patch shortly.
>
>
> Right. I actually overlooked the 'passthrough' in
> PQregisterEventProc. It turns out that we are still not quite on the
> same page and this needs to be clarified before we move on. The
> passthrough cannot exist...the correct prototypes (reasoning will
> follow) are:
>
> typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo);

small typo: eventInfo obviously can't be const

> int PQregisterEventProc(PGconn *conn, PGeventProc proc);
> PQhookData(const PGconn* conn, PGeventProc proc);

PQhookData is the old name...we are going with 'events' now....the
proper names will come with the patch.

merlin

"Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
> Right. I actually overlooked the 'passthrough' in
> PQregisterEventProc. It turns out that we are still not quite on the
> same page and this needs to be clarified before we move on. The
> passthrough cannot exist...

Yes, it *will* exist. You are assuming that the goals you have for
these hooks are the only ones anyone will ever have. There are other
possible usage patterns and many of them will need some passthrough
state data. I'd venture to say that anytime I've ever used a callback
design that did not include a passthrough, I've had reason to curse
its designer sooner or later (qsort being a pretty typical example).

However, it's clear that we are not on the same page, because what
I thought you wanted the PQeventData function to return was the
passthrough pointer. Evidently that's not what you want it to do,
so you'd better back up a few steps and say what you do want it to do.
(I think that a function to get the passthrough value associated with
a given hook function might be useful too, but it's clear that what
you are after must be something else.)

> The purpose of the callbacks is to allow a hooking library to
> establish private data that is associated with a PGconn or a PGresult.

So you're imagining that on creation of a PGconn or PGresult, the
hook function would be allowed to create some storage and libpq would
then be responsible for tracking that storage? Okay, but that's a
separate feature from the sort of passthrough I had in mind. Where
exactly does the hook hand off the storage pointer to libpq? What
are you going to do if the hook fails to create the storage
(ie, out of memory during PGresult creation)?

> Invoking PQregisterEventProc tells libpq 'I am interested in doing
> this', for the supplied PGconn, future results created with that
> connection, and (if PGconn is passed as null), all future connections.

I am entirely serious about saying that I will not accept that last bit.
To do that we have to buy into having permanent modifiable storage
within libpq, protecting it with thread mutexes, etc etc. And I don't
like any of the possible usage scenarios for it. I think hooking into a
PGconn immediately after its creation is just as useful and a lot easier
to manage.

> Once that is established, libpq begins telling the hooking library
> when and what needs to be allocated or deleted.

Wait a minute --- you're trying to get between libpq and malloc?
Why? That's getting a *whole* lot more invasive than I thought
this patch intended to be, and I don't see a good reason for it.

regards, tom lane

On Fri, May 16, 2008 at 4:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
>> Right. I actually overlooked the 'passthrough' in
>> PQregisterEventProc. It turns out that we are still not quite on the
>> same page and this needs to be clarified before we move on. The
>> passthrough cannot exist...
>
> Yes, it *will* exist. You are assuming that the goals you have for
> these hooks are the only ones anyone will ever have. There are other
> possible usage patterns and many of them will need some passthrough
> state data. I'd venture to say that anytime I've ever used a callback
> design that did not include a passthrough, I've had reason to curse
> its designer sooner or later (qsort being a pretty typical example).

right. It can exist, just not hold the event data (the extended
properties of PGresult/conn). It has a meaning and scope that are not
defined for libpq purposes. For the 'result' callbacks (see below),
we will just use the passthrough passed in for the connection.
libpqtypes will not use this, and we were getting nervous about people
understanding what the different pointers were used for (we would call
it a user pointer, not a pass through).

> However, it's clear that we are not on the same page, because what
> I thought you wanted the PQeventData function to return was the
> passthrough pointer. Evidently that's not what you want it to do,
> so you'd better back up a few steps and say what you do want it to do.
> (I think that a function to get the passthrough value associated with
> a given hook function might be useful too, but it's clear that what

We will do this:
void *PQeventData(PGconn*, PGeventProc, void **passthrough);
void *PQresultEventData(PGresult*, PGeventProc, void **passthrough);

> you are after must be something else.)

The major challenge is that libpqtypes (and, presumably, other
libraries) must essentially store its own data in both conn and
result. We allocate our data when these structures are created and
free it when they are destroyed. The idea is that libpq fires
callbacks at appropriate times when conn/results are
constructed/destructed. From these events we set up our private
'event' data and delete it. Each connection and result maintains a
'list' of private event states, one for each registering callback
(libpqtypes only requires one). So, registering an event proc is
analogous to adding one or fields to a conn or a result with a private
meaning.

So, libpq is defining 6 events:
*) when a connection is created (put into OK status)
*) when a connection is reset
*) when a connection is destroyed
*) when a result is created in non-error state
*) when a result is copied (we are adding this to libpq w/PQcopyResult)
*) when a result is destroyed

In these events we set up or tear down the private state for the libpq
objects. We need to sometimes look up the data from outside the
library in public (non callback) code. This is what PQeventData, etc.
accomplish...provide the private state for the object. The
difference is that there can be more than one registered event proc on
a conn/result at a time; thus the need for an additional "lookup"
value when getting event data (what was hookName and is now the event
proc address).

>> The purpose of the callbacks is to allow a hooking library to
>> establish private data that is associated with a PGconn or a PGresult.
>
> So you're imagining that on creation of a PGconn or PGresult, the
> hook function would be allowed to create some storage and libpq would
> then be responsible for tracking that storage? Okay, but that's a
> separate feature from the sort of passthrough I had in mind. Where
> exactly does the hook hand off the storage pointer to libpq? What
> are you going to do if the hook fails to create the storage
> (ie, out of memory during PGresult creation)?

Right, this could happen for one of two likely reasons: OOM as you
suggest or the callback fails for some reason only known to the
hooking library. In either case, the callback would set the return
pointer as defined by the structure to null, return FALSE, and libpq
would not add the state to the array of 'event state datas' it
maintains.

Here is the event proc with passthrough added:
int PGEventProc(PGEventId event, void *eventInfo, void *passThrough);
// FALSE = failure

>> Invoking PQregisterEventProc tells libpq 'I am interested in doing
>> this', for the supplied PGconn, future results created with that
>> connection, and (if PGconn is passed as null), all future connections.
>
> I am entirely serious about saying that I will not accept that last bit.
> To do that we have to buy into having permanent modifiable storage
> within libpq, protecting it with thread mutexes, etc etc. And I don't
> like any of the possible usage scenarios for it. I think hooking into a
> PGconn immediately after its creation is just as useful and a lot easier
> to manage.

Locking is not required so long as you follow certain conventions.
Here is our warning that describes this in the header:
+ * WARNING: This should only be called in the application's main thread
+ * before using any other libpq functions. This is not thread-safe and
+ * should be used prior to creating any application threads.

Obviously, the 'global' behavior is optional, but nice (it's kind of a
'gotcha' if you forget to re-register in a reconnect routine for
example). It is like an _init() routine, and considered doing it that
way. It does mean adding an array of 'global' event procs to libpq.
So I would ask you to hold your judgment on this, and, once we get the
new patch in, consider it and we will pull it if you still think it's
too dangerous (meaning, the convention is not followed, by a api
user).

>> Once that is established, libpq begins telling the hooking library
>> when and what needs to be allocated or deleted.
>
> Wait a minute --- you're trying to get between libpq and malloc?
> Why? That's getting a *whole* lot more invasive than I thought
> this patch intended to be, and I don't see a good reason for it.

No, we don't get in between. Maybe my wording was a little
misleading...libpq notifies the registered event proc implementations
of when a conn or result is created/destroyed. libpq, strictly
speaking, is not allocating the private states...just notifying other
code it's time to do it. There is no change to the way libpq
allocates things generally.

When we send up the revised patch, you will see the invasion is
minimal...the trickiest part was actually injecting the events in the
proper places.

merlin

Tom Lane wrote:
> Where
> exactly does the hook hand off the storage pointer to libpq? What
> are you going to do if the hook fails to create the storage
> (ie, out of memory during PGresult creation)?

The current submitted patch has 3 of its function callbacks returning a
void*, initHookData after the creation of a conn, resultCreate, and
resultCopy. We have recently changed this design so all hooks, now
called events, go through a single callback ... PGEventProc. The old
function callbacks are just eventIds now.

/////////////////////////////////
// The libpq side (looping registered event procs)
PGEventResultCreate info;
info.stateData = NULL; /* our event data ptr */
info.conn = conn;
info.result = result;

if(!result->evtState[i].proc(PGEVT_RESULTCREATE,
(void *)&info, result->evtState[i].passThrough)
{
PQclear(result); // destroy result? ... not sure
return error; // previously, we ignored it
}

// assign event data created by event proc.
result->evtState[i].data = info.stateData;

///////////////////////////////////////
// example of what the event proc does
int my_evtproc(PGEventId evtId, void *evtInfo, void *passThrough)
{
switch(eventId)
{
case PGEVT_RESULTCREATE:
{
void *data = makeresultdata(....)
if(!data)
return FALSE;
((PGEventResultCreate *)evtInfo)->stateData = data;
break;
}
}

return TRUE;
}

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

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

"Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
> On Fri, May 16, 2008 at 4:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> So you're imagining that on creation of a PGconn or PGresult, the
>> hook function would be allowed to create some storage and libpq would
>> then be responsible for tracking that storage? Okay, but that's a
>> separate feature from the sort of passthrough I had in mind. Where
>> exactly does the hook hand off the storage pointer to libpq? What
>> are you going to do if the hook fails to create the storage
>> (ie, out of memory during PGresult creation)?

> Right, this could happen for one of two likely reasons: OOM as you
> suggest or the callback fails for some reason only known to the
> hooking library. In either case, the callback would set the return
> pointer as defined by the structure to null, return FALSE, and libpq
> would not add the state to the array of 'event state datas' it
> maintains.

So at that point we have an apparently-successful creation of a
PGresult, but some of the hook library's functionality is going to fail
to work with that PGresult. That doesn't seem very nice. ISTM the hook
ought to be able to request that libpq return an out-of-memory failure
for the query, just as would happen if the malloc failure had happened
directly to libpq.

regards, tom lane

Tom Lane wrote:
> ISTM the hook
> ought to be able to request that libpq return an out-of-memory failure
> for the query, just as would happen if the malloc failure had happened
> directly to libpq.
>
>

I am working on this new patch and that is how I have been implementing it. If
the eventProc function returns FALSE for creation events (not destruction
events), the libpq call that triggered it will fail. For instance: for the
creation of result objects "PGEVT_RESULTCREATE", I am clearing the result and
returning an error value.

I think that is the expected behavior when one calls PQexec and an out-of-memory
failure occurs.

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

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.

I am continuing to use the object hooks thread to avoid confusing anyone.

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.

event proc - the callback procedure/function implemented outside of
libpq ... PGEventProc. The address of the event proc is used as a
lookup key for getting a conn/result's event data.

event data - the state data managed by the event proc but tracked by
libpq. This allows the event proc implementor to basically add a
dynamic struct member to a conn or result. This is an instance based
value, unlike the "arg pointer".

arg pointer - this is the passthrough/user pointer. I called it 'arg'
as other libpq callbacks used this term for this type of value. This
value can be retrieved via PQeventData, PQresultEventData ... its an
optional double pointer argument.

event state - an internal structure, PGEventState, which contains the
event data, event proc and the 'arg' pointer. Internally, PGconn and
PGresult contain arrays of event states.

event id - PGEventId enum values, PGEVT_xxx. This tells the event proc
which event has occurred.

event info - These are the structures for event ids, like
PGEVT_RESULTDESTROY has a corresponding PGEventResultDestroy structure.
The PGEventProc function's 2nd argument is "void *info". The first
argument is an event id which tells the proc implementor how to cast the
void *. This replaced the initial va_arg idea.

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

On Sat, May 17, 2008 at 8:28 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> 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.

Let's decide where to go with this. We have no objections to pushing
this back to the next fest (tom's comments on the wiki were noted).
If that's the case though, we would like to wrap up a consenus on the
general implementation in the meantime. Just give us a heads up and
I'll update the wiki.

merlin

Merlin Moncure wrote:
> On Sat, May 17, 2008 at 8:28 AM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> 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.
>
>
> Let's decide where to go with this. We have no objections to pushing
> this back to the next fest (tom's comments on the wiki were noted).
> If that's the case though, we would like to wrap up a consenus on the
> general implementation in the meantime. Just give us a heads up and
> I'll update the wiki.
>
> merlin
>
>

Yeah, it would be nice to avoid another push back into July by getting some
feedback now for June; it would be great to squeeze it into May. I'm not
talking about a review, but maybe a couple of "I hate this" or "This works well"
or "Give up coding" :) The implementation of the events (as well as when they
were object hooks) is actually very simple, so a quick glance can probably raise
most concerns. There is very little going on.

In the end, something like this can be done several different ways (choice here
is a matter of taste & style). It would be nice to iron out the API and focus
on other aspects; namely is this general concept appealing enough.

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

Andrew Chernow <ac(at)esilo(dot)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

Will make all of those changes. We appreciate the help.

1. remove global registration :(

2. New Name: PGCallback

3. use instanceData and passThrough names (passThrough with upper 'T')

3. separate getters for conn/result instanceData and passthrough

4. add a setter for result instance data
- There should also be a PQsetInstanceData(PGconn*, ...)
- I see no need for a passThrough setter

5. move callback stuff to its own header, maybe pgcallback.h?

No issue with any of them. Although, small issue below:

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

Adding PQresultSetInstanceData doesn't removes the need for a resultcreate
callback event. This is an event the callbacks are informed about (instanceData
or not). It does remove the need for an instance data member in all event info
structures, just use the getter/setter when desired. If the passThrough is
needed, one can use the public getters.

> hooks registered. Also, meseems you need such a callback anyway:
> what if the hook library desires to realloc its instance data
> larger?
>

With your suggestions, this would work:

res = PQexec(conn, "blah");
data = PQresultInstanceData(res, cbfunc);
data = realloc(data, 1024);
PQresultSetInstanceData(res, cbfunc, data);

The API user should have a valid instanceData whenever libpq returns a result,
assuming they registered a callback that allocates instance data during a
resultcreate event.

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
> 4. add a setter for result instance data
> - There should also be a PQsetInstanceData(PGconn*, ...)
> - I see no need for a passThrough setter

Check, though I assume we're not expecting PQsetInstanceData to
propagate to previously created PGresults?

> 5. move callback stuff to its own header, maybe pgcallback.h?

Should be libpq-something. I was considering libpq-hooks.h or
libpq-events.h, but libpq-callback.h would be OK too.

> Adding PQresultSetInstanceData doesn't removes the need for a resultcreate
> callback event.

No, of course not. What I was imagining was that the resultcreate
callback would call PQresultSetInstanceData.

regards, tom lane

On Mon, May 19, 2008 at 10:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Chernow <ac(at)esilo(dot)com> writes:
>> 4. add a setter for result instance data
>> - There should also be a PQsetInstanceData(PGconn*, ...)
>> - I see no need for a passThrough setter
>
> Check, though I assume we're not expecting PQsetInstanceData to
> propagate to previously created PGresults?
>
>> 5. move callback stuff to its own header, maybe pgcallback.h?
>
> Should be libpq-something. I was considering libpq-hooks.h or
> libpq-events.h, but libpq-callback.h would be OK too.

I think 'events' best describes what is going on. Your other
suggestions are improvements (we maybe tried a little too hard to
minimize exports). Losing the global hooks is no problem (it was
basically syntax sugar).

merlin

Tom Lane wrote:
> Andrew Chernow <ac(at)esilo(dot)com> writes:
>> 4. add a setter for result instance data
>> - There should also be a PQsetInstanceData(PGconn*, ...)
>> - I see no need for a passThrough setter
>
> Check, though I assume we're not expecting PQsetInstanceData to
> propagate to previously created PGresults?
>

No, not at all. Already created results are on their own. If you want to
modify the instanceData, you can use PQresultSetInstanceData.

>> 5. move callback stuff to its own header, maybe pgcallback.h?
>
> Should be libpq-something. I was considering libpq-hooks.h or
> libpq-events.h, but libpq-callback.h would be OK too.
>

I like events. It sounds like you wanted PGCallback, although I am starting to
think PGEventProc is better than PGcallback, only because it is more consistent
with the term events.

BTW, my suggestion to call this libpq events was not directly referring to the
callback/proc. It was a term for describing the whole #! I think the idea is
to notify interested parties about libpq events, the callback is just an
implementaion for doing that.

>> Adding PQresultSetInstanceData doesn't removes the need for a resultcreate
>> callback event.
>
> No, of course not. What I was imagining was that the resultcreate
> callback would call PQresultSetInstanceData.
>

Sorry, my mistake. You were actually very clear, my reading skills are in question.

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

Attached is the latest patch. It has addressed the requested changes found
here: http://archives.postgresql.org/pgsql-patches/2008-05/msg00389.php

Its a tarball because there are two new files, libpq-events.c and
libpq-events.h. The patch is in the tarball as well as attached to the email.

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

On Mon, May 19, 2008 at 8:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Chernow <ac(at)esilo(dot)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,

Interesting you haven't commented on two areas, the actual placement
of the event callers in the libpq code (i'm assuming these are ok),
and the PQcopyResult/PQsetvalue functions. The latter area introduces
some new behavior into standard libpq (imo) so it deserves some
scrutiny.

merlin

Andrew Chernow escribió:
> Attached is the latest patch. It has addressed the requested changes
> found here:
> http://archives.postgresql.org/pgsql-patches/2008-05/msg00389.php
>
> Its a tarball because there are two new files, libpq-events.c and
> libpq-events.h. The patch is in the tarball as well as attached to the
> email.

I modified this patch slightly. I was about to try libpqtypes on it,
but then I noticed that libpqtypes as published on pgfoundry is based on
a very old version of this patch, so I punted. So, for now, the only
guarantee is that it compiles with no warnings.

However, the only change of any significance that I introduced was that
a "name" is attached to every event proc, so that it can be reported in
error messages, as reporting only %p seems very useless. (I also
removed PQresultAlloc.)

The API seems reasonable to me.

There's one thing that seems a bit baroque, which is the
PG_COPYRES_USE_ATTRS stuff in PQcopyResult. I think that flag
introduces different enough behavior that it should be a routine of its
own, say PQcopyResultAttrs. That way you would leave out the two extra
params in PQcopyResult.

Oh -- one last thing. I am not really sure about the flags to
PQcopyResult. Should there really be flags to _remove_ behavior,
instead of flags that add? i.e. instead of having "0" copy everything,
and have to pass flags for things not to copy, wouldn't it be cleaner to
have 0 copy only base stuff, and require flags to copy extra things?

The main missing thing from this patch is SGML docs for the new libpq
functions.

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

Alvaro Herrera wrote:
> Andrew Chernow escribió:
>> Attached is the latest patch. It has addressed the requested changes
>> found here:
>> http://archives.postgresql.org/pgsql-patches/2008-05/msg00389.php
>>
>> Its a tarball because there are two new files, libpq-events.c and
>> libpq-events.h. The patch is in the tarball as well as attached to the
>> email.
>
> I modified this patch slightly. I was about to try libpqtypes on it,
> but then I noticed that libpqtypes as published on pgfoundry is based on
> a very old version of this patch, so I punted. So, for now, the only
> guarantee is that it compiles with no warnings.
>

Being that it is now on pgfoundry, we didn't want to make drastic changes. We
wanted to wait to see how this patch unfolds and make a major change then.

"event" version of libpqtypes (not on foundry yet)
http://libpqtypes.esilo.com/libpqtypes-events.tar.gz

> (I also removed PQresultAlloc.)
>

Nooooooo ... removing PQresultAlloc breaks libpqtypes! It also removes some of
the use cases provided by PQsetvalue, which allows one to add to a result (in
our case from scratch).

> There's one thing that seems a bit baroque, which is the
> PG_COPYRES_USE_ATTRS stuff in PQcopyResult. I think that flag
> introduces different enough behavior that it should be a routine of its
> own, say PQcopyResultAttrs. That way you would leave out the two extra
> params in PQcopyResult.
>

Okay. We can move attr copying to its own function.

> Oh -- one last thing. I am not really sure about the flags to
> PQcopyResult. Should there really be flags to _remove_ behavior,
> instead of flags that add?
>

Agreed, that is kinda weird. Probably ended up that because the flags were an
after thought.

> However, the only change of any significance that I introduced was that
> a "name" is attached to every event proc, so that it can be reported in
> error messages, as reporting only %p seems very useless. (I also
> removed PQresultAlloc.)

I don't mind re-introducing the name, but Tom seemed very against this due to
conflicts. If 2 different libraries register the same name, debugging would be
painful.

> The main missing thing from this patch is SGML docs for the
> new libpq functions.

We have no issue here. We are waiting for the patch to gain acceptance so we
only document this once.

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

Alvaro Herrera wrote:
>
> There's one thing that seems a bit baroque, which is the
> PG_COPYRES_USE_ATTRS stuff in PQcopyResult. I think that flag
> introduces different enough behavior that it should be a routine of its
> own, say PQcopyResultAttrs. That way you would leave out the two extra
> params in PQcopyResult.
>
> Oh -- one last thing. I am not really sure about the flags to
> PQcopyResult. Should there really be flags to _remove_ behavior,
> instead of flags that add? i.e. instead of having "0" copy everything,
> and have to pass flags for things not to copy, wouldn't it be cleaner to
> have 0 copy only base stuff, and require flags to copy extra things?
>
> a "name" is attached to every event proc, so that it can be
> reported in error messages
>

Can someone confirm that an event 'name' should be re-introduced, as
suggested by Alvaro?

Can I get a happy or sad face in regards to below?

New options which add instead of remove.

#define PG_COPYRES_ATTRS 0x01
#define PG_COPYRES_TUPLES 0x02 /* Implies PG_COPYRES_ATTRS */
#define PG_COPYRES_EVENTS 0x04
#define PG_COPYRES_NOTICEHOOKS 0x08

// tuples implies attrs, you need the attrs to copy the tuples.
if(options & PG_COPYRES_TUPLES)
options |= PG_COPYRES_ATTRS; // auto set option

In regards to copying the attrs, the PQcopyResult still needs the
ability to copy the source result's attrs. Although, it doesn't need
the ability to provide custom attrs (which I removed). New prototype
for copyresult:

PGresult *
PQcopyResult(const PGresult *src, int options);

I then added a PQsetResultAttrs. copyresultattrs didn't seem like the
correct name because you are no longer copying attrs from a source
result. You are providing the attrs to 'set'.

int
PQsetResultAttrs(PGresult *res, int numAttributes,
PGresAttDesc *attDescs);

If the result provided to setattrs already contains attributes, I have
the function failing (can't overwrite existing attrs). I think this is
good behavior....

When PQcopyResult needs to copy the source result's attrs, it calls
PQsetResultAttrs.

/* Wants attrs */
if((options & PG_COPYRES_ATTRS) &&
!PQsetResultAttrs(dest, src->numAttributes, src->attDescs))
{
PQclear(dest);
return NULL;
}

So, there is some nice code reuse which indicates to me the code is
segmented well (copyres & setattrs).

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

This is an updated version pf the libpqevents patch. See

http://archives.postgresql.org/pgsql-hackers/2008-09/msg00153.php

for details. The only change I didn't make yet is the event 'name'. I
have put it in and taken it out twice now, so a firm 'put it in there'
would be appreciated.

Go here for libpqtypes using events. pgfoundry is still using the older
object hooks version.

http://libpqtypes.esilo.com/libpqtypes-events.tar.gz

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

Andrew Chernow escribió:
> Alvaro Herrera wrote:

>> (I also removed PQresultAlloc.)
>
> Nooooooo ... removing PQresultAlloc breaks libpqtypes! It also removes
> some of the use cases provided by PQsetvalue, which allows one to add to
> a result (in our case from scratch).

I don't really see the point -- it's the same as pqResultAlloc, except
that you have to pass an extra argument. There's no actual
functionality loss.

> > However, the only change of any significance that I introduced was that
> > a "name" is attached to every event proc, so that it can be reported in
> > error messages, as reporting only %p seems very useless. (I also
> > removed PQresultAlloc.)
>
> I don't mind re-introducing the name, but Tom seemed very against this
> due to conflicts. If 2 different libraries register the same name,
> debugging would be painful.

Hmm, is that really a good argument? I don't see how providing a
pointer is a better answer than a name -- it's far less user friendly.
Let's start assuming that no duplicate names would be used, and if there
are any conflicts in the real world, we can just ask the user to fire up
GDB to figure out where each name is pointing to. My guess is that
conflicting names will be a rarity, if we ever get to see them. (Would
two different libraries call themselves "pqtypes", for example?)

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Andrew Chernow wrote:
> This is an updated version pf the libpqevents patch. See
>
> http://archives.postgresql.org/pgsql-hackers/2008-09/msg00153.php
>
> for details. The only change I didn't make yet is the event 'name'. I
> have put it in and taken it out twice now, so a firm 'put it in there'
> would be appreciated.

You didn't merge the other changes I did to your patch. Please use that
one as starting point, or merge them into your version. They were
minor, sure, but I went some lengths to make them, which means the code
kinda has my signoff that way. Please don't waste that work.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Andrew Chernow escribió:
>> Alvaro Herrera wrote:
>
>>> (I also removed PQresultAlloc.)
>> Nooooooo ... removing PQresultAlloc breaks libpqtypes! It also removes
>> some of the use cases provided by PQsetvalue, which allows one to add to
>> a result (in our case from scratch).
>
> I don't really see the point -- it's the same as pqResultAlloc, except
> that you have to pass an extra argument. There's no actual
> functionality loss.
>

libpqtypes uses it. libpqtypes doesn't have access to any internals of
libpq, including pqResultAlloc. So, I made a public wrapper to the
internal version. The point is to provide public access to the result
allocator.

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

Andrew Chernow wrote:
> This is an updated version pf the libpqevents patch. See
>
> http://archives.postgresql.org/pgsql-hackers/2008-09/msg00153.php
>
> for details. The only change I didn't make yet is the event 'name'. I
> have put it in and taken it out twice now, so a firm 'put it in there'
> would be appreciated.
>
> Go here for libpqtypes using events. pgfoundry is still using the older
> object hooks version.
>
> http://libpqtypes.esilo.com/libpqtypes-events.tar.gz
>
>

Patch update again. This one includes an optional event name for
debugging purposes. It also includes the changes made by Alvaro that I
missed.

PQregisterEventProc now takes a name argumet. If the name is NULL, the
error message will identify the event procedure by its address ...
"addr:%p". If its not NULL, error messages will indicate the provided name.

I updated the styling in libpq-events.c and a couple places in fe-exec.c
that Alvaro missed.

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