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

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

Andrew Dunstan wrote:
> 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. +

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

Andrew Dunstan 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.

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

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

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

On Wed, May 14, 2008 at 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

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

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

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

Merlin Moncure wrote:
> 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. +

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

On Wed, May 14, 2008 at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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

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

On Wed, May 14, 2008 at 10:52 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

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

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

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