Thread: Re: libpq object hooks (libpq events)
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.
Attachment
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 thatit 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/
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
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/