Re: libpq patch for pqtypes hook api and PGresult creation

Here are the changes to libpq. It adds the ability to register an
Object Hook and create a home-grown result. Patch adds 4 functions.

We changed the name of PQresultSetFieldValue to PQsetvalue, which better
compliments PQgetvalue. If this patch is acceptable, we will move on to
making the required changes to pqtypes; some work has already been done.

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

On Fri, Apr 11, 2008 at 1:47 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
> Here are the changes to libpq. It adds the ability to register an Object
> Hook and create a home-grown result. Patch adds 4 functions.
>
> We changed the name of PQresultSetFieldValue to PQsetvalue, which better
> compliments PQgetvalue. If this patch is acceptable, we will move on to
> making the required changes to pqtypes; some work has already been done.

Whoops! One missing thing here...we forgot to make pqResultAlloc
pubilc...pqResultAlloc -> PQresultAlloc (see discussion in -hackers).
Also, we could use pqResultStrdup (or keep it private, in which case
we can re-implement in libpqtypes).

merlin

Merlin Moncure wrote:
> On Fri, Apr 11, 2008 at 1:47 PM, Andrew Chernow <ac(at)esilo(dot)com> wrote:
>> Here are the changes to libpq. It adds the ability to register an Object
>> Hook and create a home-grown result. Patch adds 4 functions.
>>
>> We changed the name of PQresultSetFieldValue to PQsetvalue, which better
>> compliments PQgetvalue. If this patch is acceptable, we will move on to
>> making the required changes to pqtypes; some work has already been done.
>
> Whoops! One missing thing here...we forgot to make pqResultAlloc
> pubilc...pqResultAlloc -> PQresultAlloc (see discussion in -hackers).
> Also, we could use pqResultStrdup (or keep it private, in which case
> we can re-implement in libpqtypes).
>
> merlin
>

The connCreate and resultCreate hooks are in the wrong place. I didn't
realize this until I started to implement the hook functions in pqtypes.

void (*connCreate)(const char *hookName, const PGconn *conn);

The requirements for the connCreate hook are that the PGconn is ready
for use. I am currently hooking in connectDBStart, which is dead wrong.
After some poking around, it looks like the correct place to hook
would be in PQconnectPoll ... does this sound correct? There are 3
places PQconnectPoll returns PGRES_POLLING_OK: one is at the top of the
function and the other two are further down with "We are open for
business!" comments. Would I be correct to hook in at these 3 places in
PQconnectPoll?

void (*resultCreate)(const char *hookName, const PGconn *conn,
const PGresult *res);

The requirements for resultCreate are that the result is fully
constructed. I am currently hooked in PQmakeEmptyResult, again dead
wrong. Does PQgetResult sound correct?

Also, pqtypes is only interested in CONNECTION_OK and successfull
results. But, these hooks are available to more than just pqtypes.
What should the "when to call connCreate and resultCreate" policy be?
Should the hook only be called when the conn or result was successfull
or should the hooks be called for failed connections/commands as well?

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

>
> Should the hook only be called when the conn or result was
> successfull or should the hooks be called for failed
> connections/commands as well?
>

ISTM that the hooks should be called on success and error (doesn't
include cases where a NULL conn or result is returned). I think this
makes things more useful. If the hooker (pun intended) isn't interested
in error results or conns, it can easily ignore them.

connCreate: The best solution I found was to hook into PQconnectPoll.
This required making the current PQconnectPoll a static named
connectPoll and making PQconnectPoll a wrapper to it. The wrapper just
calls connectPoll and checks the status for hook points.

resultCreate: PQgetResult seemed like the best place. I only notify the
hooks if the resultStatus is NOT copyin or copyout.

I diff'd fe-connect.c and fe-exec.c against cvs which shows these changes.

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
> The requirements for the connCreate hook are that the PGconn is ready
> for use. I am currently hooking in connectDBStart, which is dead wrong.

I looked at the "object hooks" patch and it looked like a complete mess.
AFAICS the only way you could use it would be to insert hooks at library
_init() time, meaning that the mere linking of libpgtypes into an
executable would cause all your hook overhead to occur on every
connection and every query ever made by that program. The thread
locking you put in is completely horrid as well --- you've got it
holding a process-wide lock over operations that are likely to include
nontrivial database interactions.

I think you need to consider something a bit less invasive. What I'd
imagine is something more like this: a program that wishes to use
libpgtypes calls "PQinitTypes(PGconn *conn)" immediately after
establishing a connection, and that installs hooks into connection-local
storage and does whatever per-connection setup it needs. No need for
any global state nor any thread mutexes.

Lastly, as far as the hook designs themselves: the "hook name" concept
seems utterly useless, and what *is* needed is missing: every callback
function needs a pass-through void * pointer so that it can get at
private state.

regards, tom lane

Kind of a long post, but if you take the time to read it we think it accurately
clarifies how we interrupt the current objections and how we see this working.
NOTE: any references to overhead are in regards to library size, not performance.

>would be to insert hooks at library
>_init() time, meaning that the mere linking of libpgtypes

Alvaro Herrera wrote:
"Maybe there's a way we can have libpqtypes adding calls into some
hypothetical libpq hooks. So libpqtypes registers its hooks in _init()
or some such, and it gets picked up automatically by any app that links
to it."

>the "hook name" concept
Not needed anymore if we do per-conn hooks. I was doing library wide hooks, it
felt natural to allow them to be removed (ability not needed per-conn). You can
only remove hooks if you have a means of referencing what you want to remove.
From that perspective, the names served a purpose - PQremoveObjectHooks("myhook");

>you've got it holding a process-wide lock
Okay, easy change to install per-conn. I was trying to avoid having to set these
hooks on every connection.

There are some dirty details in regards to locking. Even if you remove the
locking from libpq hooks, you still incur a lock at every hook point inside
pqtypes. pqtypes has to map a conn and result (we'll call this a pqobj) to
pqtypes typeData. Adding a void* to the hook funcs doesn't help because non-hook
functions like getf, paramcreate, etc. only have a ptr to a pqobj: PQgetf(res,
..), PQparamCreate(conn, ..). Since the private storage of a pqobj is not
directly accessible, you have to either A) map pqobj addresses to typeData in a
pqtypes global array that must be locked or B) add two libpq API calls
PQtypeData(conn), PQresultTypeData(res).

> libpgtypes calls "PQinitTypes(PGconn *conn)"
As this stands, it wouldn't work. You need some hook funcptr arguments. Without
them, there is no way to communicate with pqtypes.

Tom Lane wrote:
"hooks that could be used by either libpgtypes or something that would like to
do something roughly similar"

I don't think PQinitTypes, private type data per conn/result or the need for
PQtypeData(conn), PQresultTypeData(res) (to avoid locking in pqtypes) keeps
things in line with this requirement (generic hook api). Has this requirement
changed? BTW, Tom was not the only one to suggest generic design. That's why I
came up with object hooks - notifications of libpq object states. Best
name/concept I can come up with. PQinitTypes(conn) is really
PQaddObjectHook(conn, hook) -- addition of the conn argument -- to keep it generic.

In the end, the problem is that the wrong tool "hooks" is being used for
pqtypes. Hooks normally allow a completely unrelated library to receive events.
I think we are forcing a hook design on to something that is completely
"related" (libpqtypes is an extension of libpq, getvalue and getf are siblings).
There is no need for hooks. If we wanted to add
PQsetBillingMethod(PQconn*,PQbillingMethod*), then you could make a case for
hooks (obviously the billing api doesn't fit). But that is not the case for
PQgetf, PQputf, PQparamExec, PQparamSend, ....

The argument against pqtypes being part of libpq was library size overhead.
This was verbalized by many people (issues with redhat packaging were also
brought up). I never heard a complaint about the 10 API calls we wanted to add.
Only that those 10 API calls came at a 50K library bloat cost, and there were
no buyers.

This brings me back to the dlopen idea. If you want to use pqtypes,
PQtypesLoad(). The guts of the library are in libpqtypes.so so the only thing
left in libpq are simple functions like below:

// libpq
PQparamExec(...)
{
if(libpqtypes->paramExec)//typesLoad issued dlsym calls
// we are in libpq, access to private conn storage granted
return libpqtypes->paramExec(conn, conn->typeData, ...);
return "library not loaded: call PQtypesLoad";
}

// end user
#include <libpqtypes.h> // pqtypes typedefs, includes libpq-fe.h
PQtypesLoad(); // call before using libpq
res = PQparamExec(conn, param, .....);

The library size issue is resolved. I never heard any complaints about this
approach. Andrew Dunstan said "Please make sure that any scheme you have along
these lines will work on Windows DLLs too.", which didn't sound like a complaint
to me.

#ifdef WIN32
# define dlopen(libname, flags) LoadLibraryA(libname)
# define dlsym(handle, sym) GetProcAddress(handle, sym)
#endif

Tom also weighed in but he thought I was confused about his hook idea (as the
proposed dlopen is completely different):

Tom Wrote:
"This is still 100% backwards. My idea of a libpq hook is something that
could be used by libpgtypes *and other things*. What you are proposing
is something where the entire API of the supposed add-on is hard-wired
into libpq."

He is 100% correct, the dlopen idea is 100% backwards from a hook concept. It
was not an implementation idea for the hooks concept, it was a different
approach altogether. Instead of a Hooks API, just add the pqtypes API.

What are the objections to adding 10 API calls to libpq (each around 5-10 lines,
minimal typedefs needed) and being able to dlopen behind PQtypesLoad?

hooks vs. dlopen

Hooks:
Need 5 API calls
- PQinitTypes
- PQmakeResult
- PQsetvalue
- PQtypeData - avoid locks in pqtypes
- PQresultTypeData - avoid locks in pqtypes
Need hook points in several places
Need storage in conn and result
Need a couple typedefs
Adds minimal size overhead to libpq
Two funcs are useful to other apps, makeResult & setvalue
Must expose internal type, PGresAttDesc

dlopen
Need 10 API calls
Need storage in conn and result
Need a few typedefs
Adds minimal size overhead to libpq
All funcs are useful to other apps
No internals need to be exposed

The two are not much different. Although, the hooks idea is a bit awkward when
applied to pqtypes and adds a layer of abstraction not needed.

Another important question is: What's more useful to libpq apps, a hook api
allowing you to receive created/destroyed events about libpq objects OR the
features of pqtypes (put and get any type in binary including arrays & composites)?

Playing with the idea: PQtypesLoad could take an argument specifying a
libpqtypes version (for speaking to a particular backend). Could even add a
PQtypesUnload so that you can unload+load a different version within the same
process. For now, I am just proposing a load function that should be called
before using libpq.

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