Thread: Re: [PATCHES] libpq type system 0.9a
I know there has been lots of versions and technical feedback related to this proposed feature. However, I have talked to Tom and neither of us see sufficient user request for this capability to add this code into the core server. I recommend you place it on pgfoundry and see if you can get a sufficient user-base there. If you need something exposed by libpq that is not there already, please let us know. One interesting idea would be if ecpg could use this functionality in place of its own type-specific functions --- if that were the case, we could reconsider adding this to core because it would be required by ecpg. Sorry for the bad news. I think we all hoped that enough interest would be generated for this to be accepted. --------------------------------------------------------------------------- Andrew Chernow wrote: > Andrew Chernow wrote: > > Joe Conway wrote: > >> Alvaro Herrera wrote: > >>> Merlin Moncure escribi?: > >>>> Yesterday, we notified -hackers of the latest version of the libpq > >>>> type system. Just to be sure the right people are getting notified, > >>>> we are posting the latest patch here as well. Would love to get some > >>>> feedback on this. > >>> > >>> I had a look at this patch some days ago, and the first question in my > >>> mind was: why is it explicitely on libpq? Why not have it as a separate > >>> library (say libpqtypes)? That way, applications not using it would not > >>> need to link to it. Applications interested in using it would just need > >>> to add another -l switch to their link line. > >>> > >> > >> +1 > >> > >> Joe > >> > >> > > > > What is gained by having a separate library? Our changes don't bloat > > the library size so I'm curious what the benefits are to not linking > > with it? If someone doesn't want to use, they don't have to. Similar > > to the backend, there is stuff in there I personally don't use (like geo > > types), but I'm not sure that justifies a link option -lgeotypes. > > > > The changes we made are closely tied to libpq's functionality. Adding > > PQputf to simplify the parameterized API, adding PQgetf to compliment > > PQgetvalue and added the ability to register user-defined type handlers > > (used by putf and getf). PQgetf makes extensive use of PGresult's > > internal API, especially for arrays and composites. Breaking this into > > a separate library would require an external library to access the > > private internals of libpq. > > > > Personally, I am not really in favor of this idea because it breaks > > apart code that is very related. Although, it is doable. > > > > I poked around to see how this would work. There are some problems. > > 1. members were added to PGconn so connection-based type handler information can > be copied to PGparam and PGresult objects. > > 2. members were added to PGresult, referenced in #1. To properly getf values, > the connection-based type handler information must be available to PGresult. > Otherwise, PQgetf would require an additional argument, a PGconn, which may have > been closed already. > > 3. PQfinish calls pqClearTypeHandler to free type info assigned to the PGconn. > > 4. PQclear also calls pqClearTypeHandlers > > It would also remove some of the simplicity. Creating a connection would no > longer initialized type info, which gets copied to PGparam and PGresult. Type > info includes a list of built-in handlers and backend config, like > integer_datetimes, server-version, etc... That means an additional function > must be called after PQconnectdb. But where would the type info be stored? It > wouldn't exist in PGconn anymore? Also, this would require double frees. You > have to free the result as well as the type info since they are no longer one > object. Same holds true for a pgconn. > > There is something elegant about not requiring additional API calls to perform a > putf or getf. It'll just work if you want to use it. You can use PQgetf on a > result returned by PQexec and you can use PQputf, PQparamExec followed by > PQgetvalue. > > -- > 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 -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > I know there has been lots of versions and technical feedback related to > this proposed feature. However, I have talked to Tom and neither of us > see sufficient user request for this capability to add this code into > the core server. I recommend you place it on pgfoundry and see if you > can get a sufficient user-base there. If you need something exposed by > libpq that is not there already, please let us know. > > One interesting idea would be if ecpg could use this functionality in > place of its own type-specific functions --- if that were the case, we > could reconsider adding this to core because it would be required by > ecpg. > > Sorry for the bad news. I think we all hoped that enough interest would > be generated for this to be accepted. > > I think you should conduct a wider survey before you make that decision. In particular, I'd like to hear from driver writers like Greg Sabino Mullane and Jeff Davis, as well as regular libpq users. cheers andrew
Andrew Dunstan wrote: > > > Bruce Momjian wrote: > > I know there has been lots of versions and technical feedback related to > > this proposed feature. However, I have talked to Tom and neither of us > > see sufficient user request for this capability to add this code into > > the core server. I recommend you place it on pgfoundry and see if you > > can get a sufficient user-base there. If you need something exposed by > > libpq that is not there already, please let us know. > > > > One interesting idea would be if ecpg could use this functionality in > > place of its own type-specific functions --- if that were the case, we > > could reconsider adding this to core because it would be required by > > ecpg. > > > > Sorry for the bad news. I think we all hoped that enough interest would > > be generated for this to be accepted. > > > > > > I think you should conduct a wider survey before you make that decision. > In particular, I'd like to hear from driver writers like Greg Sabino > Mullane and Jeff Davis, as well as regular libpq users. Well, they are welcome to chime in but the patch has been out the for a while and they haven't spoken yet. We have to base our decisions on people who do chime in. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Andrew Dunstan <andrew@dunslane.net> writes:
> I think you should conduct a wider survey before you make that decision.
> In particular, I'd like to hear from driver writers like Greg Sabino
> Mullane and Jeff Davis, as well as regular libpq users.
Well, the survey's already been taken, pretty much: there's been just
about no positive feedback in all the time that this proposal's been
discussed. I haven't noticed anyone except Andrew and Merlin saying
that they wanted this or would use it.
Currently there's about 400K of C source code in libpq. The patch as-is
adds more than 100K, and it would surely get larger if we actively
pursued this line of development. (For one thing, the density of
comments in the submitted patch is well below what I'd find acceptable;
by the time it was commented to a level comparable to the existing libpq
code, it'd be a lot more than 100K.) That's a pretty big increment for
a facility that it appears would be used only by a small minority of
users. I think that at minimum we'd have to insist on it being
refactored as a separate library, and then the case for it being in core
rather than on pgfoundry seems kinda weak.
regards, tom lane
-----BEGIN PGP SIGNED MESSAGE----- Hash: RIPEMD160 > I think you should conduct a wider survey before you make that decision. > In particular, I'd like to hear from driver writers like Greg Sabino > Mullane and Jeff Davis, as well as regular libpq users. I can state that there would be almost zero chance this would ever be used by DBD::Pg, as it would seem to add overhead with no additional functionality over what we already have. Unless I'm misreading what it does and someone can make the case why I should use it. - -- Greg Sabino Mullane greg@turnstep.com PGP Key: 0x14964AC8 200804081349 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -----BEGIN PGP SIGNATURE----- iEYEAREDAAYFAkf7sGkACgkQvJuQZxSWSsi8FgCgkGUGh2ieOAtvNlXX6orjO8oc 0bIAoPF7ojxM1c38kw7+L4Ar7FRZmdrn =U2BM -----END PGP SIGNATURE-----
On Tue, Apr 8, 2008 at 12:59 PM, Bruce Momjian <bruce@momjian.us> wrote: > Sorry for the bad news. I think we all hoped that enough interest would > be generated for this to be accepted. I think that's really unfortunate. Personally, I think that anyone who did any amount of C coding against libpq at all would never have any reason to code in the traditional fashion (PQexec, etc). Anyone who would claim otherwise IMO does not code vs. libpq or does not completely understand what we are trying to do. In particular, I think that the decision to so quickly shut the door on the ability to support arrays and composites in binary on the client side. Contrary to what is stated there have been considerable requests for this on the various lists. I am dismayed that throughout this process there has been no substantive discussion (save for Tom) on what we were trying to do, only to be informed about rejection based on an internal discussion. What issues were raised were opaque and sans reasoning or justification of how that would improve the patch or the functionality (move to separate library for example -- how would this improve things?). Our follow ups were not followed up. We would have been delighted to take suggestions. I attributed the silence to general lack of interest and anticipated this response. However I think that those involved should step back and take a look at what they are walking away from here. merlin
On Tue, Apr 8, 2008 at 1:51 PM, Greg Sabino Mullane <greg@turnstep.com> wrote: > > I think you should conduct a wider survey before you make that decision. > > In particular, I'd like to hear from driver writers like Greg Sabino > > Mullane and Jeff Davis, as well as regular libpq users. > > I can state that there would be almost zero chance this would ever be > used by DBD::Pg, as it would seem to add overhead with no additional > functionality over what we already have. Unless I'm misreading what it > does and someone can make the case why I should use it. does DBD::pg move arrays in and out of the server? do you parse them yourself? if you answer yes to either question you should take a second look. merlin
Merlin Moncure escribió: > I attributed the silence to general lack of interest and anticipated > this response. However I think that those involved should step back > and take a look at what they are walking away from here. I suggest you take a survey on a more widely read forum, like pgsql-general or even pgsql-announce. Keep in mind that many of the most active people in pgsql-hackers is not actually writing libpq programs (I know I am not.) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Greg Sabino Mullane wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: RIPEMD160 > > >> I think you should conduct a wider survey before you make that decision. >> In particular, I'd like to hear from driver writers like Greg Sabino >> Mullane and Jeff Davis, as well as regular libpq users. > > I can state that there would be almost zero chance this would ever be > used by DBD::Pg, as it would seem to add overhead with no additional > functionality over what we already have. Unless I'm misreading what it > does and someone can make the case why I should use it. > > - -- > Greg Sabino Mullane greg@turnstep.com > PGP Key: 0x14964AC8 200804081349 > http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 > > -----BEGIN PGP SIGNATURE----- > > iEYEAREDAAYFAkf7sGkACgkQvJuQZxSWSsi8FgCgkGUGh2ieOAtvNlXX6orjO8oc > 0bIAoPF7ojxM1c38kw7+L4Ar7FRZmdrn > =U2BM > -----END PGP SIGNATURE----- > > > This idea is for the libpq user, although driver writers could find it handy as well. Really, anyone who uses libpq directly. That's the real audience. I don't know what overhead greg is referring to. How is DBD::pg handling arrays of composites? Are you parsing text output? Wouldn't it be less overhead to avoid text parsing and transmit binary data? >>no additional functionality over what we already have What about user-defined type registration, sub-classing user or built-in type handlers, handling of all data types in binary. There is a lot of new functionality added by this patch to the existing libpq. I don't think the appropriate audience got a look at this, maybe posting on general or libpq lists. From my perspective as a long time C coder, this made my application code cleaner, easier to maintain and faster in many cases. It removed a lot of code that is now handled by this patch. I am not sure why Tom is worried about source code size, normally the concern is linked size. Code comments were never finished, as the library was changing so much to meet some requests. Instead, we focused on providing API documentation and the overall idea (over 1000 lines). This changed much less than the implementation. I think the real issue is simply the wrong audience. Its the coder in the field making heavy use of libpq that would find this appealing, not really backend hackers. It is disappointing because I was excited to here ideas from others, which never happened. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
"Merlin Moncure" <mmoncure@gmail.com> writes:
> On Tue, Apr 8, 2008 at 1:51 PM, Greg Sabino Mullane <greg@turnstep.com> wrote:
>> I can state that there would be almost zero chance this would ever be
>> used by DBD::Pg, as it would seem to add overhead with no additional
>> functionality over what we already have. Unless I'm misreading what it
>> does and someone can make the case why I should use it.
> does DBD::pg move arrays in and out of the server? do you parse them
> yourself? if you answer yes to either question you should take a
> second look.
Better support for arrays and composites is certainly something that
people might want, but the problem with this design is that it forces
them to buy into a number of other decisions that they don't necessarily
want.
I could see adding four functions to libpq that create and parse
the textual representations of arrays and records.
regards, tom lane
On Tue, 08 Apr 2008 14:34:51 -0400 Andrew Chernow <ac@esilo.com> wrote: > I am not sure why Tom is worried about source code size, normally the > concern is linked size. Code comments were never finished, as the Every byte added is a byte maintained (or not). Joshua D. Drake -- The PostgreSQL Company since 1997: http://www.commandprompt.com/ PostgreSQL Community Conference: http://www.postgresqlconference.org/ United States PostgreSQL Association: http://www.postgresql.us/ Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
Tom Lane wrote: > Better support for arrays and composites is certainly something that > people might want, but the problem with this design is that it forces > them to buy into a number of other decisions that they don't necessarily > want. > > I could see adding four functions to libpq that create and parse > the textual representations of arrays and records. > > > Well, that was the part that interested me, so let me now speak up in favor of better array/record support in libpq. cheers andrew
On Tue, Apr 8, 2008 at 2:49 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > > Better support for arrays and composites is certainly something that > > people might want, but the problem with this design is that it forces > > them to buy into a number of other decisions that they don't necessarily > > want. > > > > I could see adding four functions to libpq that create and parse > > the textual representations of arrays and records. > Well, that was the part that interested me, so let me now speak up in favor > of better array/record support in libpq. by the way, we handle both text and binary array results...and getting things in binary is _much_ faster. not to mention text is destructive. for example, composite types in text do not return the oid of composite member fields. with our patch, since you can 'pop' a result of a returned composite, or array of composite, you have access to all that information in the result api. so I would argue that allowing text only parsing only recovers a portion of the provided functionality. merlin
Tom Lane wrote: > > Better support for arrays and composites is certainly something that > people might want, but the problem with this design is that it forces > them to buy into a number of other decisions that they don't necessarily > want. > > > regards, tom lane > What decisions are we forcing upon the libpq user? Well, most of the functionality is handled by about 3 functions (putf, getf, and paramexec). The difference is, our patch is not limited to only handling text arrays and composites. It can do it all, which we thought would of been a requirement to get approved. There is a performance boost to handling arrays and composites in binary, which we use a lot because there are no stored procedures (note, not trying to take a jab about stored procedures, just giving an example of how we use and abuse arrays and composites). -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
"Joshua D. Drake" <jd@commandprompt.com> writes:
> On Tue, 08 Apr 2008 14:34:51 -0400
> Andrew Chernow <ac@esilo.com> wrote:
>> I am not sure why Tom is worried about source code size, normally the
>> concern is linked size. Code comments were never finished, as the
> Every byte added is a byte maintained (or not).
Actually I was thinking more about disk footprint. Andrew's comment is
correct if you work with statically linked code where the compiler pulls
out only the needed .o files from a .a library, but that's pretty out of
fashion these days. Most people are dealing with a monolithic libpq.so
and might carp a bit if it gets 25% or 50% bigger for stuff that doesn't
interest them.
Perhaps I'm overly sensitive to this because I'm tuned into Red Hat's
constant struggles to fit a Linux distribution onto a reasonable number
of CDs ...
regards, tom lane
Merlin Moncure wrote: > I attributed the silence to general lack of interest and anticipated > this response. However I think that those involved should step back > and take a look at what they are walking away from here. Agreed. There are technical issues, but they can be addressed with work. The more fundamental issue is whether we want this functionality in libpq vs pgfoundry, and with a lack of interest, as you stated, having it on pgfoundry seems the only logical direction. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: > > On Tue, 08 Apr 2008 14:34:51 -0400 > > Andrew Chernow <ac@esilo.com> wrote: > >> I am not sure why Tom is worried about source code size, normally the > >> concern is linked size. Code comments were never finished, as the > > > Every byte added is a byte maintained (or not). > > Actually I was thinking more about disk footprint. Andrew's comment is > correct if you work with statically linked code where the compiler pulls > out only the needed .o files from a .a library, but that's pretty out of > fashion these days. Most people are dealing with a monolithic libpq.so > and might carp a bit if it gets 25% or 50% bigger for stuff that doesn't > interest them. > > Perhaps I'm overly sensitive to this because I'm tuned into Red Hat's > constant struggles to fit a Linux distribution onto a reasonable number > of CDs ... Also, if we add to libpq we have to document this new functionality. It doesn't make sense to add to the API unless there is a significant number of people who will use it. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Andrew Chernow <ac@esilo.com> writes:
> Tom Lane wrote:
>> Better support for arrays and composites is certainly something that
>> people might want, but the problem with this design is that it forces
>> them to buy into a number of other decisions that they don't necessarily
>> want.
> What decisions are we forcing upon the libpq user?
Well, for starters, using binary format. It is undeniable that that
creates more portability risks (cross-architecture and cross-PG-version
issues) than text format. Not everyone wants to take those risks for
benefits that may not be meaningful for their apps.
The other forced decision is the whole PQputf/PQgetf notation, which
people may or may not find natural --- it still seems a pretty poor
choice to me for this specific problem. PQputf, in the form where
you're generating a SQL command string along with some parameters,
isn't too unreasonable, but unless you've already bought into binary
parameter handling it's not gaining all that much either. And
PQgetf is just weird. The format of a PGresult is generally well
known by the app; trying to force it into the model of string
scanning is a poor fit. For instance there's no mapping at all
for what to do with constant parts of the format string.
regards, tom lane
On Tue, Apr 8, 2008 at 3:10 PM, Bruce Momjian <bruce@momjian.us> wrote: > > Actually I was thinking more about disk footprint. Andrew's comment is > > correct if you work with statically linked code where the compiler pulls > > out only the needed .o files from a .a library, but that's pretty out of > > fashion these days. Most people are dealing with a monolithic libpq.so > > and might carp a bit if it gets 25% or 50% bigger for stuff that doesn't > > interest them. on my box, the .so went from 118k to 175k. while this is significant in percentage terms, I don't think redhat is going to complain about 57k (correct me if I'm wrong here). > Also, if we add to libpq we have to document this new functionality. It > doesn't make sense to add to the API unless there is a significant > number of people who will use it. We are prepared to write the formal documentation if necessary (and whatever else, code comments and a proper regression test for example). I'll address the 'who will want to use it' in response to Tom's mail. merlin
On Tue, Apr 8, 2008 at 3:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Chernow <ac@esilo.com> writes: > > Tom Lane wrote: > >> Better support for arrays and composites is certainly something that > >> people might want, but the problem with this design is that it forces > >> them to buy into a number of other decisions that they don't necessarily > >> want. > > What decisions are we forcing upon the libpq user? > > Well, for starters, using binary format. It is undeniable that that > creates more portability risks (cross-architecture and cross-PG-version > issues) than text format. Not everyone wants to take those risks for > benefits that may not be meaningful for their apps. personally, I think these claims of portability are a bit overblown...they are trivially checked by regressions tests. the version question is more interesting and solutions range from the easy (force version match to access binary types) to the complex. > The other forced decision is the whole PQputf/PQgetf notation, which > people may or may not find natural --- it still seems a pretty poor > choice to me for this specific problem. PQputf, in the form where > you're generating a SQL command string along with some parameters, > isn't too unreasonable, but unless you've already bought into binary > parameter handling it's not gaining all that much either. And > PQgetf is just weird. The format of a PGresult is generally well > known by the app; trying to force it into the model of string > scanning is a poor fit. For instance there's no mapping at all > for what to do with constant parts of the format string. We chose to do the PQgetf function that way as a compromise of many different requirements (we went through several alternative implementations before settling on the final one). At minimum, you have to supply at least _some_ type information because getf writes into application memory, and aside from the obvious safety issues, there are user defined types to consider. Since you can register additional type into the handler system at runtime (including 'automagic' discovery of composite types by name), there has to be some way of directing the library to the proper handler. Since everything is driven by typename, it all comes down to how you want to do this. We like the varargs style since it provides some symmetry with putf and feels 'right' in coding. The only other approach to getf that meets all the requirements would be like this: PQgetf(result, tup_num, "date", 0, &date); // not varargs where the typestring is fixed to one type...you could only pull out one parameter at a time (no variable argument list). We considered this carefully, and we opted for the submitted approach as the best solution. One other benefit to doing things this way is we get to alter the 'format escape character, (%)', to # when lookup by name as opposed to column position is desired. The constant part of the format string in getf are ignored, they have no meaning in that context. Same with putf. Although, there is PQputvf which will take something like this: "select %int4 + %int4" and turn it into "select $1 + $2". In PQputvf, everything surrounding the type modifier is still ignored. So, the API is symmetrical in terms of type string, except that for query execution there is an extra stage where the query string is mapped. What you find to be weird here I think is rather elegant :-). >>PQputf/PQgetf notation, which > people may or may not find natural --- it still seems a pretty poor > choice to me for this specific problem We reference types by there name (can include schema as well). There is no better mnemonic than that ... is there? For example, if I have some funky type 'foo' defined on the server, what is more natural than pulling that type with %foo (which can be optionally schema qualified)? By the way, we ended up with the current implementation based on your concerns with previous attempts (using two letter format codes, or exported static type handlers)...(we really like how things turned out though...using schema qualified type names as the 'typespec' really makes the libpq code look 'pretty'. merlin
On Tue, Apr 08, 2008 at 02:34:51PM -0400, Andrew Chernow wrote: > This idea is for the libpq user, although driver writers could find it > handy as well. Really, anyone who uses libpq directly. That's the real > audience. Quite, I'm writing array parsing code right now and this would make my life much easier too. However, my question is: > I am not sure why Tom is worried about source code size, normally the > concern is linked size. How tight is the link to libpq? Could it exist as a seperate library: libpqbin or something? Still in core, just only used by the people who want it. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Please line up in a tree and maintain the heap invariant while > boarding. Thank you for flying nlogn airlines.
Martijn van Oosterhout wrote: -- Start of PGP signed section. > On Tue, Apr 08, 2008 at 02:34:51PM -0400, Andrew Chernow wrote: > > This idea is for the libpq user, although driver writers could find it > > handy as well. Really, anyone who uses libpq directly. That's the real > > audience. > > Quite, I'm writing array parsing code right now and this would make my > life much easier too. However, my question is: > > > I am not sure why Tom is worried about source code size, normally the > > concern is linked size. > > How tight is the link to libpq? Could it exist as a seperate library: > libpqbin or something? Still in core, just only used by the people who > want it. The idea of pgfoundry was that it would be an independent library and could be used by people who need it. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Martijn van Oosterhout wrote: > How tight is the link to libpq? Could it exist as a seperate library: > libpqbin or something? Still in core, just only used by the people who > want it. > I gave this a lot of thought and I do think we could abstract this. The idea is to complie it in or out. Add a --with-typesys to configure, which could enable "#ifdef LIBPQ_ENABLE_TYPESYS" everywhere. If you don't specify --with-typesys, the API calls would still be there but would return ENOSYS, assign an error string or something. This preserves link capatability. This would trim out the 50k everyone is worried about :) I'm sure there are other ways to acocmplish this, but this one seems easiest and keeps it all centralized. Just like --with-openssl, except that loads libcrypto.so. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > Martijn van Oosterhout wrote: >> How tight is the link to libpq? Could it exist as a seperate library: >> libpqbin or something? Still in core, just only used by the people who >> want it. >> > > I gave this a lot of thought and I do think we could abstract this. The > idea is to complie it in or out. > > Add a --with-typesys to configure, which could enable "#ifdef > LIBPQ_ENABLE_TYPESYS" everywhere. If you don't specify --with-typesys, > the API calls would still be there but would return ENOSYS, assign an > error string or something. This preserves link capatability. > > This would trim out the 50k everyone is worried about :) I'm sure there > are other ways to acocmplish this, but this one seems easiest and keeps > it all centralized. Just like --with-openssl, except that loads > libcrypto.so. > Forgot to say: There is stuff in PGconn, PGresult, PQclear, PQfinish (maybe a couple other places). A separate library would remove the ability to call PQexec followed by PQgetf because the result object is no longer aware of the typesys. You would need the separate library to wrap the result object or something: typesysResult = TypeSysGetResult(PQexec()); Or, you need to wrap the libpq API calls, typesysResult = TypeSysExec();. Both are doable but not nearly as slick as: res = PQexec; PQgetf(res, ..); PQclear(res); -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
"Merlin Moncure" <mmoncure@gmail.com> writes:
>>> Actually I was thinking more about disk footprint. Andrew's comment is
>>> correct if you work with statically linked code where the compiler pulls
>>> out only the needed .o files from a .a library, but that's pretty out of
>>> fashion these days. Most people are dealing with a monolithic libpq.so
>>> and might carp a bit if it gets 25% or 50% bigger for stuff that doesn't
>>> interest them.
> on my box, the .so went from 118k to 175k. while this is significant
> in percentage terms, I don't think redhat is going to complain about
> 57k (correct me if I'm wrong here).
Yipes, 48% growth in the binary already? That's much higher than
I'd expected from my quick source-code count, even with the scarcity of
comments. What happens by the time the feature actually becomes mature?
Yes, I could see Red Hat complaining about that. I'd rather have libpq
(as it currently stands) included in core RHEL, and "libpqtypes" as an
optional extra, than have a monolithic library get booted out to extras
altogether. It could happen. Don't think they don't see us as a
secondary objective ... mysql still has a lot more mindshare.
regards, tom lane
Andrew Chernow <ac@esilo.com> writes:
>> I gave this a lot of thought and I do think we could abstract this. The
>> idea is to complie it in or out.
[shrug...] So the packagers will compile it out, and you're still hosed,
or at least any users who'd like to use it are.
> Forgot to say: There is stuff in PGconn, PGresult, PQclear, PQfinish
> (maybe a couple other places).
I didn't see anything there that seemed that it *had* to be inside
libpq, and certainly not anything that couldn't be handled with a
small hook or two.
regards, tom lane
Bruce Momjian wrote: > Martijn van Oosterhout wrote: > > How tight is the link to libpq? Could it exist as a seperate library: > > libpqbin or something? Still in core, just only used by the people who > > want it. > > The idea of pgfoundry was that it would be an independent library and > could be used by people who need it. I don't think phasing it out to pgfoundry is a good idea, because it has some dependency on the OIDs of datatypes. Besides, it is likely that it could be used by ecpg instead of it having its own PGTYPES stuff. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Tom Lane wrote:
> Andrew Chernow <ac@esilo.com> writes:
>>> I gave this a lot of thought and I do think we could abstract this. The
>>> idea is to complie it in or out.
>
> [shrug...] So the packagers will compile it out, and you're still hosed,
> or at least any users who'd like to use it are.
>
>> Forgot to say: There is stuff in PGconn, PGresult, PQclear, PQfinish
>> (maybe a couple other places).
>
> I didn't see anything there that seemed that it *had* to be inside
> libpq, and certainly not anything that couldn't be handled with a
> small hook or two.
>
> regards, tom lane
>
How about using dlopen and dlsym?
Sseparate the library as many are suggesting. But leave the functions, the
tid-bits in PGconn, PGresult, etc... in there (2 or 3 typedefs would be needed
but all data-type typedefs "PGtimestamp" can be removed). You need something
inside the PGconn and PGresult, even if just a function pointer that gets called
if not NULL.
Yank pqtypes function bodies, and related helper functions, and replace them
with something like below:
int PQputf(...)
{
#ifdef HAVE_DLOPEN if(pqtypes->putf) return pqtypes->putf(...); return 1; /* failed, PGparam error = "not enabled"
*/
#else return 1; /* failed, PGparam error = "cannot load dynamically" */
#endif
}
Then add a PQtypesEnable(bool), which would dlopen(libpqtypes) and dlsym the 10
functions or so we need. Any typedefs you need would be in libpqtypes.h rather
than libpq-fe.h. You could disable it as well, which would unload libpqtypes.
The default would obviously be disabled.
The entire patch would be one small file with a couple 1 line changes to PGconn
and PGresult. This would remove all overhead, at least 95% of it.
>>couldn't be handled with a small hook or two.
Maybe, have not thought of that. The problem, is that I am trying to make avoid
having to keep track of two different APIs. The goal is the libpq user is
coding to the libpq API, not some other API like PGTypesExec.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Andrew Chernow wrote: > Forgot to say: There is stuff in PGconn, PGresult, PQclear, PQfinish > (maybe a couple other places). 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. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Andrew Chernow wrote: > >> Forgot to say: There is stuff in PGconn, PGresult, PQclear, PQfinish >> (maybe a couple other places). > > 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. > Kinda what my last suggestion was. Some tid-bits need to be reside in libpq, but very little. I was thinking PQtypesEnable(bool) which would dlopen libpqtypes and map all functions needed. This would leave the function bodies of PQputf, PQgetf, PQparamExec, etc... as simple proxy functions to the dynamically loaded functions. This removes any bloat that people don't like right now but still allows one to use libpq as the primary interface, rather than having to fiddle with libpq and some other API. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > Alvaro Herrera wrote: >> Andrew Chernow wrote: >> >>> Forgot to say: There is stuff in PGconn, PGresult, PQclear, >>> PQfinish (maybe a couple other places). >> >> 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. >> > > Kinda what my last suggestion was. Some tid-bits need to be reside in > libpq, but very little. I was thinking PQtypesEnable(bool) which > would dlopen libpqtypes and map all functions needed. This would > leave the function bodies of PQputf, PQgetf, PQparamExec, etc... as > simple proxy functions to the dynamically loaded functions. This > removes any bloat that people don't like right now but still allows > one to use libpq as the primary interface, rather than having to > fiddle with libpq and some other API. Please make sure that any scheme you have along these lines will work on Windows DLLs too. cheers andrew
Andrew Dunstan wrote: > > Please make sure that any scheme you have along these lines will work on > Windows DLLs too. > > Ofcourse: LoadLibrary(), GetProcAddress(), __declspec(dllexport). -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Tue, Apr 8, 2008 at 6:09 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Andrew Chernow wrote: > > > Forgot to say: There is stuff in PGconn, PGresult, PQclear, PQfinish > > (maybe a couple other places). > > 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. I've been having some thoughts along those lines as well. As currently written there is a already a de facto 'init' as the various static type handlers are assembled into the type handlers for the built in types. About 50% of the patch as written is libpq plumbing which extends the API, defines the structures, and various things like that. IMO, there is very little point to abstracting that out into a separate library. The 50% is the type handlers and various assorted conversion functions. This half will only increase as we introduce new types and other conversions. I think, if there is some reasonable case for separation, it is here (the type handlers)...and I think this might present a reasonable approach for dealing with version compatibility issues. One thought I had was that a 8.4 libpq would be able to load the 8.3 types when dealing with a 8.3 server for example. Maybe this is a non-starter, but it's one case where splitting things out might have some other advantages. If this works the way I'm thinking about it, it would probably better than a compile time flag...I don't think that satisfies anyway since hardly anyone compiles their own libpq any more (presumably everyone would just compile it in, making the check moot). Anyone with an appreciation for irony and a long memory will recall me griping about the odbc driver compiling openssl in, because it required me to package in a bunch of extra dlls on windows :-). In terms of moving the code to pgfoundry, I think this is absolutely the wrong thing to do, except in terms of deciding the patch unsuitable for inclusion into core (or deferring that decision). People that used it would naturally expect changes to happen in concert with the server. Better just to come to a consensus...in, or out :-) merlin
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Bruce Momjian wrote:
>> The idea of pgfoundry was that it would be an independent library and
>> could be used by people who need it.
> I don't think phasing it out to pgfoundry is a good idea, because it has
> some dependency on the OIDs of datatypes.
Well, if you'll remind me of the last time we changed the OID of a
standard datatype, I'd put some credence in that argument.
> Besides, it is likely that it
> could be used by ecpg instead of it having its own PGTYPES stuff.
Yeah, Bruce and I were talking about that, but on reflection I'm not
sure that there's much potential commonality. The thing that's most
problematic about ecpg is that it wants to offer client-side equivalents
of some server datatype-manipulation functions; and I don't actually see
much of any of that in the proposed patch. All that's really here is
format conversion stuff, so there's no hope of unifying that code
unless this patch adopts ecpg's choices of client-side representation
(which I believe are mostly Informix legacy choices, so I'm not sure we
want that).
regards, tom lane
Andrew Chernow <ac@esilo.com> writes:
> Kinda what my last suggestion was. Some tid-bits need to be reside in libpq,
> but very little. I was thinking PQtypesEnable(bool) which would dlopen
> libpqtypes and map all functions needed. This would leave the function bodies
> of PQputf, PQgetf, PQparamExec, etc... as simple proxy functions to the
> dynamically loaded functions. This removes any bloat that people don't like
> right now but still allows one to use libpq as the primary interface, rather
> than having to fiddle with libpq and some other API.
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. That's just bad design, especially when the adequacy of
the proposed API is itself in question.
When I say I'd accept some hooks into libpq, I mean some hooks that
could be used by either libpgtypes or something that would like to do
something roughly similar but with a different API offered to clients.
The particular hook that you seem to mostly need is the ability to
allocate some private memory associated with a particular PGconn object,
and maybe also with individual PGresults, and then to be able to free
that at PQclear or PQfinish. Try designing it like that.
regards, tom lane
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >> Kinda what my last suggestion was. Some tid-bits need to be reside in libpq, >> but very little. I was thinking PQtypesEnable(bool) which would dlopen >> libpqtypes and map all functions needed. This would leave the function bodies >> of PQputf, PQgetf, PQparamExec, etc... as simple proxy functions to the >> dynamically loaded functions. This removes any bloat that people don't like >> right now but still allows one to use libpq as the primary interface, rather >> than having to fiddle with libpq and some other API. > > 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. That's just bad design, especially when the adequacy of > the proposed API is itself in question. > > When I say I'd accept some hooks into libpq, I mean some hooks that > could be used by either libpgtypes or something that would like to do > something roughly similar but with a different API offered to clients. > The particular hook that you seem to mostly need is the ability to > allocate some private memory associated with a particular PGconn object, > and maybe also with individual PGresults, and then to be able to free > that at PQclear or PQfinish. Try designing it like that. > > regards, tom lane > My idea was not a response to your hook idea. It was different altogether. My idea is trying to create one interface where some parts need to be enabled (nothing wrong with that design, this is a plugin-like model). Your idea creates two interfaces where one of them can hook into the other. These are two different concepts. the question is, are you asking for two different interfaces or are you just looking to minimize overhead. I thought it was the latter which is why I proposed a plugin-like model. It removes bloat as well as maintains a single interface. Nothing wrong with the design unless it doesn't meet your requirements. Andrew
Tom Lane 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. That's just bad design, especially when the adequacy of > the proposed API is itself in question. > > When I say I'd accept some hooks into libpq, I mean some hooks that > could be used by either libpgtypes or something that would like to do > something roughly similar but with a different API offered to clients. > The particular hook that you seem to mostly need is the ability to > allocate some private memory associated with a particular PGconn object, > and maybe also with individual PGresults, and then to be able to free > that at PQclear or PQfinish. Try designing it like that. Agreed. A minimal change to libpq has a much better chance of being accepted. Let me remind people that the community is not required to accept any patch --- it is accepted based on community feedback. If we accepted everything our API would be a mess and Postgres would be much harder to use. I think a wise thing would be for the patch submitters to give a _basic_ outline of what PQparam is trying to accomplish --- I mean like 10-20 lines with a code snippet, or code snippet with/withouth PQparam. Right now there are too many people trying to guess. Of course, this should have been done at the start. I found this posting from August, 2007 but it isn't short/clear enough: http://archives.postgresql.org/pgsql-hackers/2007-08/msg00630.php I feel this API is foreign enough from what people expect from a typical database interface library that it should be a separate library with its own documentation, whether the library is a separate directory in core or in pgfoundry. FYI, it might be interesting to extend it to cover what ecpg wants --- we have been looking for a way to get the database-dependent parts of ecpg out of the ecpg directory and this might be a solution, _even_ if it makes your library larger. Again, I don't think we have trouble with another library, assuming it does something that many people want to do and it is so tied to the backend that it needs to be in core. (However, Tom's mention that the OIDs don't change weakens the logic that is should be in core.) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Andrew Chernow wrote: > My idea was not a response to your hook idea. It was different > altogether. > > My idea is trying to create one interface where some parts need to be > enabled (nothing wrong with that design, this is a plugin-like model). > > Your idea creates two interfaces where one of them can hook into the > other. These are two different concepts. > > the question is, are you asking for two different interfaces or are you > just looking to minimize overhead. I thought it was the latter which is > why I proposed a plugin-like model. It removes bloat as well as > maintains a single interface. Nothing wrong with the design unless it > doesn't meet your requirements. The idea is that the hooks you need in libpq would be available always, for your interface and for others. This would allow your library to use native libpq on any >=8.4 platform. People who want to use your library would have to link in your library and call your additional functions. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Andrew Chernow wrote: >> >> When I say I'd accept some hooks into libpq, I mean some hooks that >> could be used by either libpgtypes or something that would like to do >> something roughly similar but with a different API offered to clients. >> The particular hook that you seem to mostly need is the ability to >> allocate some private memory associated with a particular PGconn object, >> and maybe also with individual PGresults, and then to be able to free >> that at PQclear or PQfinish. Try designing it like that. >> >> regards, tom lane Your method would work as well. The only issue is you still have the same issue of binary distributed libpqs. Would redhat distribute a binary linked with libpqtypes? If not, you have the same issue of the end-user having to compile libpq ... passing -lpqtypes to the linker. If redhat did link it, you run into the disk space complaint all over again. My suggestion was trying to work around this by dynamically loading the library, PQtypesEnable(TRUE). In this model, redhat doesn't even have to distribute libpqtypes.so (considering the disk space issue). It could be easily be an additional download. All you need are some proxy functions inside libpq, PQputf calling a dynamically loaded function. This passes the disk space complaints and doesn't require a re-compile if an end-user wants to use it. Andrew
Andrew Chernow wrote: > Andrew Chernow wrote: >>> >>> When I say I'd accept some hooks into libpq, I mean some hooks that >>> could be used by either libpgtypes or something that would like to do >>> something roughly similar but with a different API offered to clients. >>> The particular hook that you seem to mostly need is the ability to >>> allocate some private memory associated with a particular PGconn >>> object, >>> and maybe also with individual PGresults, and then to be able to free >>> that at PQclear or PQfinish. Try designing it like that. >>> >>> regards, tom lane > > Your method would work as well. The only issue is you still have the > same issue of binary distributed libpqs. Would redhat distribute a > binary linked with libpqtypes? If not, you have the same issue of the > end-user having to compile libpq ... passing -lpqtypes to the linker. > If redhat did link it, you run into the disk space complaint all over > again. > > My suggestion was trying to work around this by dynamically loading > the library, PQtypesEnable(TRUE). In this model, redhat doesn't even > have to distribute libpqtypes.so (considering the disk space issue). > It could be easily be an additional download. All you need are some > proxy functions inside libpq, PQputf calling a dynamically loaded > function. This passes the disk space complaints and doesn't require a > re-compile if an end-user wants to use it. > > Why would RedHat need to know anything at all about libpqtypes? AIUI Tom is suggesting an API that in libpq that libpqtypes or some other library could use, but not that libpq would be linked against libpqtypes at all. cheers andrew
Andrew Chernow wrote: > Your method would work as well. The only issue is you still have the > same issue of binary distributed libpqs. Would redhat distribute a > binary linked with libpqtypes? If not, you have the same issue of the > end-user having to compile libpq ... passing -lpqtypes to the linker. > If redhat did link it, you run into the disk space complaint all over again. > > My suggestion was trying to work around this by dynamically loading the > library, PQtypesEnable(TRUE). In this model, redhat doesn't even have > to distribute libpqtypes.so (considering the disk space issue). It > could be easily be an additional download. All you need are some proxy > functions inside libpq, PQputf calling a dynamically loaded function. > This passes the disk space complaints and doesn't require a re-compile > if an end-user wants to use it. I don't see requiring users to add -lpqtypes to use these functions as a problem. The idea is that the default libpq would have enough hooks that you could use it without modification. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
-----BEGIN PGP SIGNED MESSAGE----- Hash: RIPEMD160 Andrew states: > What about user-defined type registration, sub-classing user or built-in > type handlers, handling of all data types in binary. There is a lot > of new functionality added by this patch to the existing libpq. All of which may be useful, and may not. Right now DBD::Pg is trying to move further from libpq, rather than binding closer to it, mainly because it's a huge dependency burden and it takes forever to add new features. For example, if this got accepted and put into 8.4, the very earliest DBD::Pg could take advantage of the code would be mid 2009 (assuming an optimistic release schedule). Even then, only once 8.4 was widely in use (2010?) would the average DBD::Pg be compiled against it. And that means lots more forking to handle the old version until then. But don't take my non-use personally: as you say, this may be more helpful towards C programmers. I only weighed in because I was asked to specifically. > I don't think the appropriate audience got a look at this, maybe posting > on general or libpq lists. From my perspective as a long time C coder, > this made my application code cleaner, easier to maintain and faster in > many cases. It removed a lot of code that is now handled by this patch. Did you even post on -interfaces? I would think that would be the first place to post, certainly before -patches. Merlin asks: > does DBD::pg move arrays in and out of the server? do you parse them > yourself? if you answer yes to either question you should take a > second look. Yes, we move them into and out of the server, and into and out of Perl arrays in the process. I'd be happy to give the new way a try however: have you any sample code and documentation? Emphasis on the latter. - -- Greg Sabino Mullane greg@turnstep.com PGP Key: 0x14964AC8 200804082025 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -----BEGIN PGP SIGNATURE----- iEYEAREDAAYFAkf8DcIACgkQvJuQZxSWSshk/QCfS8Ocsoqo6U/LPNhnz9OH9Bm4 83MAoOvsSWkKuyE+LA1UhLpyX0bICQbZ =2XsW -----END PGP SIGNATURE-----
Bruce Momjian wrote: > > I don't see requiring users to add -lpqtypes to use these functions as a > problem. The idea is that the default libpq would have enough hooks > that you could use it without modification. > You are correct, brain fart on my part. Not sure where my mind was at but I scratch those commit about redhat linking in libpqtypes. Sorry about that. I think the hook idea would work. Have to look at composites and arrays, they create their own result objects from scratch. Andrew
>> I think a wise thing would be for the patch submitters to give a _basic_> outline of what PQparam is trying to accomplish--- I mean like 10-20> lines with a code snippet, or code snippet with/withouth PQparam.>I found this posting fromAugust, 2007 but> it isn't short/clear enough:> That is very old. There are tons of examples if you download the patch and look at some of the documentation .txt files. >> FYI, it might be interesting to extend it to cover what ecpg wants ---> we have been looking for a way to get the database-dependentparts of> ecpg out of the ecpg directory and this might be a solution, _even_ if> it makes your librarylarger.> I am not all to familiar with ecpg. Our idea is nothing more than data type converters, there are no type operators. Our goal is to leverage the binary parameterized API and offer the ability to get C types from an enhanced PQgetvalue, PQgetf. PQgetf understands the external binary format of the backend, so it can convert that to C types. It also understand how to convert text output from the backend. This allows existing applications using text results to drop in PQgetf here and there. You don't have to use PGparam or PQputf at all to use PQgetf. PQputf, allows one to pack parameters into a PQparam object that can be executed at another time. It knows how to take C types and convert them to the backend's external binary format. The patch always sends data in binary format, but can get results in text or binary. Along the way, we devised a way for user-defined types to be used. This introduced PQregisterTypeHandler. This function can allow one to sub-class existing type handlers, like extending "%timestamp" and making "%epoch". It allows registering user-defined types. the type will be looked up in the backend and resolved properly. It also allows one to create aliases .. simple domains. Let's say you have a C type, typedef int user_id;. You could register an alias where "user_id=int4". Now you can putf and getf "%user_id". this abstracts code from possible integer width changes, you just change your typedef and registration code to "user_id=int8". Take a peak at the documentation files in the patch, root of tar *.txt files. they are very verbose and have loads of examples. The regression-test.c file has lots of use cases. latest: http://www.esilo.com/projects/postgresql/libpq/typesys-0.9b.tar.gz To sum it up, the main concepts are PQputf, PQgetf and PQregisterTypeHandler. The other functions maintain a PGparam or exec/send one. -- Andrew Chernow eSilo, LLC http://www.esilo.com/
On Tue, 2008-04-08 at 13:08 -0400, Andrew Dunstan wrote: > I think you should conduct a wider survey before you make that decision. > In particular, I'd like to hear from driver writers like Greg Sabino > Mullane and Jeff Davis, as well as regular libpq users. > I looked into this today. * Arrays and composites Better ability in libpq to parse and construct arrays and composites would be helpful. I think this is worthwhile to consider, and I would expose the functionality (at least for arrays) in ruby-pg if available. * Binary formatting The exclusive use of binary formats is worrisome to me. This circumvents one level of indirection that we have (i.e. that everything moves through in/out functions), and will impose a backwards-compatibility requirement on the internal representation of data types, including user-defined data types. As far as I know, we currently have no such requirement, even for built-in types. * Type conversion callbacks The type conversion hooks make some sense, but I have reservations about that policy as well. The data types in PostgreSQL will never match perfectly the data types in a host language -- NULL in particular doesn't have a direct counterpart. If there were a perfect mapping of types, it would seem like a much better idea. The problem is that we don't want to have some types that are perfectly mapped (e.g. int, bytea), some that are imperfectly mapped (e.g. dates, numeric), and some types that have no conversion defined and fall back to something else. In this case, the result format is always binary, so we can't even fall back to text. In my experience trying to implicitly map to types doesn't save a lot of time for the developer. You end up spending time referencing the documentation (or some other part of the code) to figure out how the edge cases are being handled rather than just handling them explicitly in the code. For instance, wrapping a value you expect to be a date in Date.parse() is more readable in most cases. Regards,Jeff Davis
This patch has an identity crisis. We initially called it PGparam (possibly mispelled several times as PQparam) and then changed it to libpq type system (typesys). Several on patches started to call it libpqtypes, even I did. Any objections to fixing the name to libpqtypes? Any thoughts on the hooking suggested by Tom? It sounds like it should be generic enough so more than just libpqtypes can make use of it. I think something of this nature should have input before I do anything. Possible Hook points: (at least ones needed by libpqtypes) conn_create conn_reset conn_destroy result_create result_destroy I guess libpqtypes would have to maintain a map of conns and results? Right now it can associate type info because we added members to conn and result. When conn_create(conn) is called, libpqtypes would need to map this by pointer address (as it is all it has as an identifier). Still feels like maybe there should be a void* in a conn and result used for per-connection/result based info (libpqtypes or not). -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Tue, 2008-04-08 at 12:59 -0400, Bruce Momjian wrote: > If you need something exposed by > libpq that is not there already, please let us know. The things that I found missing in libpq so far are: * The ability to choose some result columns to be binary-formatted and others to be text-formatted. I haven't thought of a reasonable API for this yet, particularly since we already have so many ways of executing a statement. * an "escapeIdent" function. * PQescapeBytea escapes for both inclusion in a SQL statement and also the escaping for the input function for bytea. This means that, if you are passing a binary value as a text-format parameter, using PQescapeBytea is incorrect, and you need to write your own octal escape routine. This is obviously a minor complaint, and may not even be worth polluting the namespace. I only mention it because it seems like an easy mistake to make. I don't have a proposal ready for any of these yet, but this is what I've found to be awkward or limiting during my work on ruby-pg. If I have a more complete proposal I'll start a new thread. Regards,Jeff Davis
On Tue, Apr 8, 2008 at 8:06 PM, Bruce Momjian <bruce@momjian.us> wrote: > I think a wise thing would be for the patch submitters to give a _basic_ > outline of what PQparam is trying to accomplish --- I mean like 10-20 > lines with a code snippet, or code snippet with/withouth PQparam. Right > now there are too many people trying to guess. Of course, this should > have been done at the start. I found this posting from August, 2007 but > it isn't short/clear enough: see [dated jan 8]: http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg102719.html merlin
On Tue, Apr 8, 2008 at 9:28 PM, Jeff Davis <pgsql@j-davis.com> wrote: with proposed changes, (I think) all your suggestions are addressed/moot. see: > * The ability to choose some result columns to be binary-formatted and > others to be text-formatted. I haven't thought of a reasonable API for > this yet, particularly since we already have so many ways of executing a > statement. with PQgetf, you are abstracted from resultformat. The library converts your data for you into consistent types (in practice, the result format is always binary)...without the 'negatives' of dealing with binary data. > * an "escapeIdent" function. not sure what this is... > * PQescapeBytea escapes for both inclusion in a SQL statement and also > the escaping for the input function for bytea. This means that, if you > are passing a binary value as a text-format parameter, using > PQescapeBytea is incorrect, and you need to write your own octal escape > routine. This is obviously a minor complaint, and may not even be worth > polluting the namespace. I only mention it because it seems like an easy > mistake to make. PQescapeBytea is unnecessary with proposed patch...input parameters are moved to proper format by the library. You simply %bytea the parameter and let the library handle the rest. merlin
On Tue, 2008-04-08 at 15:22 -0400, Tom Lane wrote: > Well, for starters, using binary format. It is undeniable that that > creates more portability risks (cross-architecture and cross-PG-version > issues) than text format. Not everyone wants to take those risks for > benefits that may not be meaningful for their apps. What are the cross-architecture risks involved? Regards,Jeff Davis
Jeff Davis wrote: > On Tue, 2008-04-08 at 15:22 -0400, Tom Lane wrote: >> Well, for starters, using binary format. It is undeniable that that >> creates more portability risks (cross-architecture and cross-PG-version >> issues) than text format. Not everyone wants to take those risks for >> benefits that may not be meaningful for their apps. > > What are the cross-architecture risks involved? > > Regards, > Jeff Davis > > >>> What are the cross-architecture risks involved? We didn't run into any issues here. close attention was paid to byte ordering, the rest was handled by libpq's cross-platform handling. cross-PG-version is a different story. the patch already uses server version to toggle (like use int4 for money pre-8.3). If we make this a separate library, it would be easy to plug in a newer or older one. Merlin had some ideas here ... Merlin? Andrew
On Tue, Apr 8, 2008 at 9:21 PM, Jeff Davis <pgsql@j-davis.com> wrote: > I looked into this today. > > * Arrays and composites > > Better ability in libpq to parse and construct arrays and composites > would be helpful. I think this is worthwhile to consider, and I would > expose the functionality (at least for arrays) in ruby-pg if available. Right..that was the principle objective. In our particular case we move a lot of data in and out via arrays, and in certain case moving data in and out via binary is a huge performance win. > * Binary formatting > > The exclusive use of binary formats is worrisome to me. This circumvents > one level of indirection that we have (i.e. that everything moves > through in/out functions), and will impose a backwards-compatibility > requirement on the internal representation of data types, including > user-defined data types. As far as I know, we currently have no such > requirement, even for built-in types. This is a valid concern. That said, the in/out functions don't change _that_ much, and even if they did..there are solutions to this problem. Forwards compatibility is the worst case (8.4 libpq connecting to 8.5 server). If this problem was reasonably addressed though, would it alleviate your concerns? > * Type conversion callbacks > > The type conversion hooks make some sense, but I have reservations about > that policy as well. The data types in PostgreSQL will never match > perfectly the data types in a host language -- NULL in particular > doesn't have a direct counterpart. > If there were a perfect mapping of types, it would seem like a much > better idea. The problem is that we don't want to have some types that > are perfectly mapped (e.g. int, bytea), some that are imperfectly mapped > (e.g. dates, numeric), and some types that have no conversion defined > and fall back to something else. In this case, the result format is > always binary, so we can't even fall back to text. We introduced types for which there were no native counterparts...PGtime for example, This choice of which types map and which don't is fairly arbitrary, but not terrible from libpq perspective. We could for example typedef int to PGint, or PGint4 to make things more consistent, I suppose. I would suggest that if you want text from the database, cast it as such in the query and pull it with %text. > In my experience trying to implicitly map to types doesn't save a lot of > time for the developer. You end up spending time referencing the > documentation (or some other part of the code) to figure out how the > edge cases are being handled rather than just handling them explicitly > in the code. For instance, wrapping a value you expect to be a date in > Date.parse() is more readable in most cases. There are many types where this is not the case. Consider the geometric types for example. There is little reason to pull these in text and farily large performance benefits to retrieving in binary. merlin
Jeff Davis <pgsql@j-davis.com> writes:
> On Tue, 2008-04-08 at 15:22 -0400, Tom Lane wrote:
>> Well, for starters, using binary format. It is undeniable that that
>> creates more portability risks (cross-architecture and cross-PG-version
>> issues) than text format. Not everyone wants to take those risks for
>> benefits that may not be meaningful for their apps.
> What are the cross-architecture risks involved?
The biggie is floating-point format. IEEE standard is not quite
universal ... and even for platforms that fully adhere to that standard,
it's not entirely clear that we get the endianness issues correct.
There used to be platforms where FP and integer endianness were
different; is anyone sure that's no longer the case?
But I'll agree that cross-version hazards are a much more clear and
present danger. We've already broken binary compatibility at least once
since the current binary-I/O system was instituted (intervals now have
three fields not two) and there are obvious candidates for future
breakage, such as text locale/encoding support.
regards, tom lane
On Tue, Apr 8, 2008 at 10:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > The biggie is floating-point format. IEEE standard is not quite > universal ... and even for platforms that fully adhere to that standard, > it's not entirely clear that we get the endianness issues correct. > There used to be platforms where FP and integer endianness were > different; is anyone sure that's no longer the case? If somebody can provide a machine to test this, we will code for this...and hope to catch any other cases with the regression test. As it happens, we have a fairly large array of old hardware to test on, pa-risc, power, mips, etc. We are still setting them up to test some of our own stuff, but we will put things through the ringer, no doubt (we will also put some machines on the buildfarm). > But I'll agree that cross-version hazards are a much more clear and > present danger. We've already broken binary compatibility at least once > since the current binary-I/O system was instituted (intervals now have > three fields not two) and there are obvious candidates for future > breakage, such as text locale/encoding support. There are other cases, for example money getting promoted to 64 bit. Here is an even better example. When looking at the array_send/recv functions, I noticed that a good candidate for wire format optimization would be to trim out the length columns for arrays where the type is fixed length and has no nulls (great win for arrays of ints and such). This change would involve modifying some of the most complicated parts of the proposed patch...these things are going to happen.. So, this problem has to be dealt with, so lets look at some ways of handling it: *) only allow direct binary mapping between client/server between same version, or 'trusted' version. (lame, but easy). *) as above, but break it down to type level. similar to catalog bump for types. basically, if the client is not the same version as the server, there is a negotiation so that the client and server agree which types if any are unsafe to send. Better, and still fairly easy, since we are still sidestepping backwards compatibility, but in a more fine-grained way. *) as above, but start (from here on in), building in logical to grok older versions of binary formats, to a certain suitable time frame. This is the biggest headache in terms of long term maintenance, since it puts lots of checks into the code, but is the only way of completely isolating clients from the issue. We have already chosen this path for a couple of times (money for example), as things evolved over the 8.3 cycle. While format changes do happen, this is a farily uncommon thing. I suspect that on average there about 3-5 format changes per major release...this would ad up to about 20 or so version specific checks in the in/out procedures should we go down the compatibility road. Not great, but not completely terrible either. In terms of everything else we had to do, this was a minor nuisance at worst. We are completely open to how this particular issue should be tackled. We didn't feel particularly rushed, since the biggest problems wouldn't manifest until 8.5 at the earliest... Another issue that you have not addressed, is that (rightly or wrongly), we are adding another non-trivial step in terms of introducing new built in types. It's already a fairly large checklist as it is (enums acquired send/recv almost too late for the 8.3 cycle).One of my least favorite things about the proposalis the amount code duplication in libpq and the server...I think that this can be fixed in the long run, eliminating this 'extra' step by consolidating the client and server in/out routines into a consolidated library. merlin
Andrew Chernow wrote:
>
> Any thoughts on the hooking suggested by Tom? It sounds like it should
> be generic enough so more than just libpqtypes can make use of it. I
> think something of this nature should have input before I do anything.
>
> Possible Hook points: (at least ones needed by libpqtypes)
> conn_create
> conn_reset
> conn_destroy
> result_create
> result_destroy
>
> I guess libpqtypes would have to maintain a map of conns and results?
> Right now it can associate type info because we added members to conn
> and result. When conn_create(conn) is called, libpqtypes would need to
> map this by pointer address (as it is all it has as an identifier).
> Still feels like maybe there should be a void* in a conn and result used
> for per-connection/result based info (libpqtypes or not).
>
Well, I can get it working with a very small patch. We actually don't need very
much in libpq. Although, making it somehow generic enough to be useful to other
extensions is a bit tricky. Please, suggestions would be helpful.
Below is a raw shell of an idea that will work for libpqtypes. Start by
removing our entire patch and then add the below:
// libpqtypes only needs the below. could add op_reset,
// op_linkerror, etc...
enum
{ HOOK_OP_CREATE, HOOK_OP_DESTROY
};
struct pg_conn
{ // everything currently in a pg_conn // ...
// libpqtypes needs HOOK_OP_DESTROY, a ptr to hookData // is always used in case the hooklib needs to allocate //
orreallocate the hookData. void *hookData; int (*connHook)(PGconn *conn, int op, void **hookData);
}
struct pg_result
{ // everything currently in a pg_result .....
// libpqtypes needs create & destroy // conn is NULL for destroy void *hookData; int (*resultHook)(PGconn *conn,
PGresult*result, int op, void **hookData);
}
freePGconn(PGconn *conn)
{ // ... if(conn->connHook) conn->connHook(conn, HOOK_OP_DESTROY, &conn->hookdata); // ...
}
PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
{ // ... (result allocated here) if(result->resultHook) result->resultHook(conn, result, HOOK_OP_CREATE,
&result->hookData); // ...
}
PQclear(PGresult *result)
{ // ... if(result->resultHook) result->resultHook(NULL, result, HOOK_OP_DESTROY, &result->hookdata); //
...
}
// library wide
int PQregisterHooks(connHook, resultHook);
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
>
> Well, I can get it working with a very small patch. We actually don't
> need very much in libpq. Although, making it somehow generic enough to
> be useful to other extensions is a bit tricky. Please, suggestions
> would be helpful.
>
> Below is a raw shell of an idea that will work for libpqtypes. Start by
> removing our entire patch and then add the below:
>
> // libpqtypes only needs the below. could add op_reset,
> // op_linkerror, etc...
> enum
> {
> HOOK_OP_CREATE,
> HOOK_OP_DESTROY
> };
>
> struct pg_conn
> {
> // everything currently in a pg_conn
> // ...
>
> // libpqtypes needs HOOK_OP_DESTROY, a ptr to hookData
> // is always used in case the hooklib needs to allocate
> // or reallocate the hookData.
> void *hookData;
> int (*connHook)(PGconn *conn, int op, void **hookData);
> }
>
> struct pg_result
> {
> // everything currently in a pg_result
> .....
>
> // libpqtypes needs create & destroy
> // conn is NULL for destroy
> void *hookData;
> int (*resultHook)(PGconn *conn, PGresult *result,
> int op, void **hookData);
> }
>
There is no need to pass hookData to the hook function. libpqtypes already
accesses PGconn and PGresult directly so it can just access the hookData member.
int (*connHook)(PGconn *conn, int op);
int (*resultHook)(PGconn *conn, PGresult *result, in top);
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
On Tue, 2008-04-08 at 22:15 -0400, Tom Lane wrote: > The biggie is floating-point format. IEEE standard is not quite > universal ... and even for platforms that fully adhere to that standard, > it's not entirely clear that we get the endianness issues correct. > There used to be platforms where FP and integer endianness were > different; is anyone sure that's no longer the case? > These seem solvable with platform-specific code in the send/recv for the applicable types. I don't have such machines to test on, however. > But I'll agree that cross-version hazards are a much more clear and > present danger. We've already broken binary compatibility at least once > since the current binary-I/O system was instituted (intervals now have > three fields not two) and there are obvious candidates for future > breakage, such as text locale/encoding support. Is there a lower standard for maintaining compatibility for external binary representations than external text representations? It seems that there would have to be more flexibility in changing the external binary representation if a type between versions in order to "be cheap to convert to internal form" as the docs say here: http://www.postgresql.org/docs/8.3/static/sql-createtype.html Regards,Jeff Davis
On Tue, 2008-04-08 at 22:06 -0400, Merlin Moncure wrote: > This is a valid concern. That said, the in/out functions don't change > _that_ much, and even if they did..there are solutions to this > problem. Forwards compatibility is the worst case (8.4 libpq > connecting to 8.5 server). If this problem was reasonably addressed > though, would it alleviate your concerns? > You're starting to convince me on this point, but the instability of external binary representations between versions is still worrisome. > We introduced types for which there were no native > counterparts...PGtime for example, This choice of which types map > and which don't is fairly arbitrary, but not terrible from libpq > perspective. We could for example typedef int to PGint, or PGint4 to > make things more consistent, I suppose. > > I would suggest that if you want text from the database, cast it as > such in the query and pull it with %text. There are many cases that are fairly hard to get a perfect mapping. If you use PGtime, for instance, there are no C operators for it, so you're basically just getting the data in binary format. That can be a benefit in many applications, but that's an argument for allowing binary-format results, not for implicitly casting from one type to another. If you try to use the C time type instead, you will have many more operators available, but will lose precision. That's OK for many applications, but not a general solution. And I think NULL is still particularly tricky. If you have to check each field explicitly anyway, it doesn't seem to save a lot of effort to implicitly cast the value. In ruby-pg, I return nil for a NULL, and I think that works fairly well because almost any operator will immediately fail on nil (in Ruby), so mistakes are caught quickly. If we're casting NULL to 0, and then have to check to see that it's NULL, that seems as error-prone as libpq is currently. I think the heart of this problem is that the mapping works great except where the types don't quite match up. To remember what types fit and how well they fit, you have to refer back to other code or docs. To really make the types match up well would be a huge effort. > > In my experience trying to implicitly map to types doesn't save a lot of > > time for the developer. You end up spending time referencing the > > documentation (or some other part of the code) to figure out how the > > edge cases are being handled rather than just handling them explicitly > > in the code. For instance, wrapping a value you expect to be a date in > > Date.parse() is more readable in most cases. > > There are many types where this is not the case. Consider the > geometric types for example. There is little reason to pull these in > text and farily large performance benefits to retrieving in binary. > Agreed. Again though, that's an argument for allowing binary-format results. Regards,Jeff Davis
>>There are many cases that are fairly hard to get a perfect mapping. If>>you use PGtime, for instance, there are no C
operatorsfor it
>
>
Yeah, our patch is designed to allow one to interface with libpq using C data
types, rather than strings (most common) or for the brave external binary format.
>>If you try to use the C time type instead, you will>>have many more operators available, but will lose precision.
That is why we didn't use something like struct tm. Instead, we have PGtime,
PGdate and PGtimestamp. PGtimestamp has a PGtime and PGdate in it. See
libpq-fe.h inside our patch.
/* Below is a PGtime. */
typedef struct
{ /* The number of hours past midnight, in the range 0 to 23. */ int hour;
/* The number of minutes after the hour, in the range 0 to 59. */ int min;
/* The number of seconds after the minute, in the range 0 to 59. */ int sec;
/* The number of microseconds after the second, in the * range of 0 to 999999. */ int usec;
/* * When non-zero, this is a TIME WITH TIME ZONE. Otherwise, * it is a TIME WITHOUT TIME ZONE. */ int
withtz;
/* A value of 1 indicates daylight savings time. A value of 0 indicates * standard time. A value of -1 means
unknownor could not determine. */ int isdst;
/* Seconds east of UTC. This value is not always available. It is * set to 0 if it cannot be determined. */ int
gmtoff;
/* Timezone abbreviation: such as EST, GMT, PDT, etc. This value is * not always available. It is set to an empty
stringif it cannot be * determined. It can also be in ISO 8601 GMT+/-hhmmss format. */ char tzname[16];
} PGtime;
Our patch was not designed to operate on these data types, although it could
with a little work. It sounds like people would like this functionality, some
referring to ecpg.
Numeric is a the odd ball in our patch. We saw no benefit to supplying a struct
for it so we always expose numerics as strings (put or get). Internally, we
convert the string to binary format for puts. On binary gets, we convert the
external format to a numeric string. We left it to the libpq user to strtod or
use a Big Number library.
>>And I think NULL is still particularly tricky
PQgetisnull is used by our code. It doesn't error out, but after zeroing the
provided user memory, it just returns. The libpq user should check isnull where
they care about it.
Almost all of the types that required a PGstruct, are very solid mappings. For
instance, geo types are straight forward, not much to a PGpoint. Geo type
structs were kept very close to the backend.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
"Jeff Davis" <pgsql@j-davis.com> writes: > * Binary formatting > > The exclusive use of binary formats is worrisome to me. This circumvents > one level of indirection that we have (i.e. that everything moves > through in/out functions), and will impose a backwards-compatibility > requirement on the internal representation of data types, including > user-defined data types. As far as I know, we currently have no such > requirement, even for built-in types. This is actually incorrect. Binary I/O still goes through a function call, the send/recv functions instead of the in/out functions. In theory these are also supposed to be cross-platform. In practice they are, uhm, less cross-platform. For example they send floats as raw (presumably) IEEE floats. There have also been fewer clients using binary mode, so you're more likely to run into bugs. But the reason fewer clients use binary mode is because they would have to implement all this type-specific knowledge. It doesn't make sense to do it for every driver or application but if you're implementing it once in a library it does start to make more sense. Note however that not every data type will necessarily provide a binary send/recv function. The built-in data types do, but only as a matter of policy. It's not an error to create a type with no binary i/o functions. So I think you have to support using text mode as an option. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's RemoteDBA services!
On Wed, Apr 9, 2008 at 6:13 AM, Gregory Stark <stark@enterprisedb.com> wrote:
> > The exclusive use of binary formats is worrisome to me. This circumvents
> > one level of indirection that we have (i.e. that everything moves
> > through in/out functions), and will impose a backwards-compatibility
> > requirement on the internal representation of data types, including
> > user-defined data types. As far as I know, we currently have no such
> > requirement, even for built-in types.
>
> This is actually incorrect. Binary I/O still goes through a function call, the
> send/recv functions instead of the in/out functions. In theory these are also
> supposed to be cross-platform.
>
> In practice they are, uhm, less cross-platform. For example they send floats
> as raw (presumably) IEEE floats. There have also been fewer clients using
> binary mode, so you're more likely to run into bugs.
small correction: you _were_ more likely to run into bugs. in the
process of developing this patch during the 8.3 cycle, we caught,
fixed, and submitted a number of binary format related issues,
including some wacky things that are not possible through the text
parser (a polygon with zero points for example). we used regression
testing heavily in development.
> But the reason fewer clients use binary mode is because they would have to
> implement all this type-specific knowledge. It doesn't make sense to do it for
> every driver or application but if you're implementing it once in a library it
> does start to make more sense.
>
> Note however that not every data type will necessarily provide a binary
> send/recv function. The built-in data types do, but only as a matter of
> policy. It's not an error to create a type with no binary i/o functions.
> So I think you have to support using text mode as an option.
You have this option. there is nothing keeping you from using getvalue
vs. getf. However, due to libpq limitations, if any datatype must
return text the entire result must be text (resultFormat)...this is
also interestingly true for functions that return 'void'. So, at
present, to use PQgetf, you result set must be binary.
In some of the early versions of the patch we introduced parsing code
to compute text results in addition to binary results (so for
getf('%int), the library does the atoi for you, checking range and
such). We ended up dropping this because we were getting nervous
about the size of the patch at that point.
In any event, I think a better solution would be to change
resultformat to mean 'give me binary format if it's available' on a
per column basis...libpq already has a method to check field format of
the result and getf can just raise a run-time error if you try to pass
a native type in when it's not available. This would be a pretty
amazing sequence of events...only likely to occur when developing new
types for example. Is this (mixed text/binary results) possible with
the v3 protocol?
merlin
Merlin Moncure wrote: > However, due to libpq limitations, if any datatype must > return text the entire result must be text (resultFormat)...this is > also interestingly true for functions that return 'void'. So, at > present, to use PQgetf, you result set must be binary. > > > I'm surprised you didn't try to address that limitation. cheers andrew
On Wed, Apr 9, 2008 at 8:04 AM, Andrew Dunstan <andrew@dunslane.net> wrote: \> Merlin Moncure wrote: > > > However, due to libpq limitations, if any datatype must > > return text the entire result must be text (resultFormat)...this is > > also interestingly true for functions that return 'void'. So, at > > present, to use PQgetf, you result set must be binary. > > I'm surprised you didn't try to address that limitation. whoops! we did...thinko on my part. Text results are fully supported save for composites and arrays. merlin
Andrew Dunstan wrote: > > > Merlin Moncure wrote: >> However, due to libpq limitations, if any datatype must >> return text the entire result must be text (resultFormat)...this is > > I'm surprised you didn't try to address that limitation. > > That would change the existing behavior of resultFormat, although not terribly. Currently, the server will spit back anerror if you use binary results but some type hasn't implemented a send/recv. Instead of an error, the server could "fallback" to the type's in/out routines and mark the column as text format. I think the "fallback" approach is more intelligent behavior but such a change could break libpq clients. They might be blindly ASSuming if the exec worked with resultFormat=1, that everything returned by PQgetvalue will be binary (I'm guilty of this one, prior to libpqtypes). Our patch would work with no changes because it supports text and binary results. So, each type handler already toggles itself based on PQfformat. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > Andrew Dunstan wrote: >> >> >> Merlin Moncure wrote: >>> However, due to libpq limitations, if any datatype must >>> return text the entire result must be text (resultFormat)...this is >> >> I'm surprised you didn't try to address that limitation. >> >> > > That would change the existing behavior of resultFormat, although not > terribly. Currently, the server will spit back an error if you use > binary results but some type hasn't implemented a send/recv. Instead > of an error, the server could "fallback" to the type's in/out routines > and mark the column as text format. > > I think the "fallback" approach is more intelligent behavior but such > a change could break libpq clients. They might be blindly ASSuming if > the exec worked with resultFormat=1, that everything returned by > PQgetvalue will be binary (I'm guilty of this one, prior to libpqtypes). > > Our patch would work with no changes because it supports text and > binary results. So, each type handler already toggles itself based on > PQfformat. That makes it sound more like a protocol limitation, rather than a libpq limitation. Or am I misunderstanding? cheers andrew
Merlin Moncure wrote: > On Wed, Apr 9, 2008 at 8:04 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > \> Merlin Moncure wrote: >>> However, due to libpq limitations, if any datatype must >>> return text the entire result must be text (resultFormat)...this is >>> also interestingly true for functions that return 'void'. So, at >>> present, to use PQgetf, you result set must be binary. >> I'm surprised you didn't try to address that limitation. > > > whoops! we did...thinko on my part. Text results are fully supported > save for composites and arrays. > > merlin > Yeah, currently composites and arrays only support binary results in libpqtypes. This forces any array elementType or anymember of a composite to have a send/recv routine. Using the "fallback to text output" approach, this limitation on array elements and composite members would be removed. >>That makes it sound more like a protocol limitation, rather than a>>libpq limitation. Or am I misunderstanding? It looks like libpq, message 'T' handling in getRowDescriptions, reads a separate format for each column off the wire. The protocol already has this ability. The backend needs a ?minor? adjustment to make use of the existing capability. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Tue, Apr 08, 2008 at 07:18:31PM -0400, Tom Lane wrote: > sure that there's much potential commonality. The thing that's most > problematic about ecpg is that it wants to offer client-side equivalents > of some server datatype-manipulation functions; and I don't actually see > much of any of that in the proposed patch. All that's really here is > format conversion stuff, so there's no hope of unifying that code > unless this patch adopts ecpg's choices of client-side representation ecpg for instance does not use a struct for time, so there doesn't seem to be an advantance for ecpg in switching. On the other side there's a disadvantage in that you can binary transfer right now given that you're on the same architecture. But if the datatypes differ this would be lost. > (which I believe are mostly Informix legacy choices, so I'm not sure we > want that). Not really. ecpg's pgtypeslib uses the same datatypes as the backend uses (well, mostly because numeric was changed in the backend after the creation of pgtypeslib). Then there's a compatibility layer that maps Informix functions and datatypes to our functions and datatypes. Michael -- Michael Meskes Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
Andrew Chernow wrote: > > Yeah, currently composites and arrays only support binary results in > libpqtypes. This forces any array elementType or any member of a > composite to have a send/recv routine. Using the "fallback to text > output" approach, this limitation on array elements and composite > members would be removed. > Actually, I am confusing the way the protocol handles arrays and composites (as a single column value, vs. the way libpqtypes handles these (as a separate result object). for instance, the members of a composite inherit the format of the column within the protocol. To allow one member of a composite to be text formatted and another be binary, would require a change to the protocol, an additional format value per member header. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: >> >> Well, I can get it working with a very small patch. We actually don't >> need very much in libpq. Although, making it somehow generic enough >> to be useful to other extensions is a bit tricky. Please, suggestions >> would be helpful. >> > Quick question on the hook concept before I try to supply a new patch. From my experience, redhat normally compiles everything into their packages; like apache modules. Why would libpq be any different in regards to libpqtypes? If they don't distribute libpqtypes, how does a libpq user link with libpqtypes? They don't have the library. Where would they get a libpqtypes.so that is compatible with redhat's supplied libpq.so? The core of what I am trying to ask is, there doesn't appear to be an advantage to separating libpqtypes from libpq in terms of space. If redhat follows their normal policy of include all (probably to make their distro as feature rich out-of-the-box as possible), then they would distribute libpqtypes.so which would use the same amount of space as if it were part of libpq. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > Andrew Chernow wrote: >>> >>> Well, I can get it working with a very small patch. We actually >>> don't need very much in libpq. Although, making it somehow generic >>> enough to be useful to other extensions is a bit tricky. Please, >>> suggestions would be helpful. >>> >> > > Quick question on the hook concept before I try to supply a new patch. > > From my experience, redhat normally compiles everything into their > packages; like apache modules. Why would libpq be any different in > regards to libpqtypes? > > If they don't distribute libpqtypes, how does a libpq user link with > libpqtypes? They don't have the library. Where would they get a > libpqtypes.so that is compatible with redhat's supplied libpq.so? > > The core of what I am trying to ask is, there doesn't appear to be an > advantage to separating libpqtypes from libpq in terms of space. If > redhat follows their normal policy of include all (probably to make > their distro as feature rich out-of-the-box as possible), then they > would distribute libpqtypes.so which would use the same amount of space > as if it were part of libpq. > By the way, I offered up the idea of compiling our patch in or out, ./configure --with-pqtypes. But Tom had said >>[shrug...] So the packagers will compile it out, and you're>>still hosed, or at least any users who'd like to use it are. If redhat would compile out our patch, no questions asked, why would they distribute libpqtypes.so. Meaning, separating it out doesn't seem to unhose us :) -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > Andrew Chernow wrote: >>> >>> Well, I can get it working with a very small patch. We actually >>> don't need very much in libpq. Although, making it somehow generic >>> enough to be useful to other extensions is a bit tricky. Please, >>> suggestions would be helpful. >>> >> > > Quick question on the hook concept before I try to supply a new patch. > > From my experience, redhat normally compiles everything into their > packages; like apache modules. Why would libpq be any different in > regards to libpqtypes? > > If they don't distribute libpqtypes, how does a libpq user link with > libpqtypes? They don't have the library. Where would they get a > libpqtypes.so that is compatible with redhat's supplied libpq.so? > > The core of what I am trying to ask is, there doesn't appear to be an > advantage to separating libpqtypes from libpq in terms of space. If > redhat follows their normal policy of include all (probably to make > their distro as feature rich out-of-the-box as possible), then they > would distribute libpqtypes.so which would use the same amount of > space as if it were part of libpq. They would get it the same way they would get anything else that uses libpq that isn't packaged with it (e.g. DBD::Pg). They would either get the package that contains it, if it exists, or get the source and build it. The package with the dependent library might belong in the extras collection, while Tom's goal (and it's a good one) is to keep libpq in the core collection. I don't get what you're not seeing about this. cheers andrew
Andrew Chernow wrote: > The core of what I am trying to ask is, there doesn't appear to be an > advantage to separating libpqtypes from libpq in terms of space. If > redhat follows their normal policy of include all (probably to make > their distro as feature rich out-of-the-box as possible), then they > would distribute libpqtypes.so which would use the same amount of space > as if it were part of libpq. My guess is that if we provide an useful library, Redhat will distribute it some way or another. In the worst case (i.e. Redhat does not distribute it at all), it will still be available on PGDG rpm repositories. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Andrew Dunstan wrote: > > I don't get what you're not seeing about this. > > > All I am trying to say is, redhat's core packages are normally very inclusive. Like apache, which includes many/most modules in the core package. I am still not convinced there is merit to a separate library. But honestly, I'm indifferent at this point. If the community wants it this way, whether or not I agree with the reasons, then consider it done. It would be helpful to get some feedback about what the requirements are for a hooking mechanism for libpqtypes: 1. Do we make the hooking system generic, for other library to use? 2. If #1 is YES, then does this hooking system need to allow registering multiple hooks per app instance. Meaning, should we implement a table of callbacks/hooks? Like a linked list of them. 3. What design is preferred? *) A single msg proc that uses a msg object which is either a big union or something that gets up casted. *) A separate function pointer per hook, like conn_create, conn_destroy *) A combo, where conn hooks are handled by one funcptr and result hooks by another. But each only has one function. Please think carefully about question #1, as this could lead to over-design or guess-design. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
> There is no need to pass hookData to the hook function. libpqtypes already
> accesses PGconn and PGresult directly so it can just access the hookData member.
That's a habit you'd really be best advised to stop, if you're going to
be a separate library. Otherwise, any time we make an internal change
in the PGconn struct, it'll break your library --- and we have and will
feel free to do that without an external soname change.
What parts of PGconn/PGresult do you need to touch that aren't exposed
already?
regards, tom lane
Andrew Chernow wrote: > Andrew Dunstan wrote: >> >> I don't get what you're not seeing about this. >> >> >> > > All I am trying to say is, redhat's core packages are normally very > inclusive. Like apache, which includes many/most modules in the core > package. > There are plenty of modules that they don't include, e.g. mod_fastcgi. If you want that you download and build it against your installed apache. Or you get mod_fcgid from extras. Your bolt-on client library would be in exactly the same boat as one of those. cheers andrew
Andrew Dunstan wrote: > > All I am trying to say is, redhat's core packages are normally very > > inclusive. Like apache, which includes many/most modules in the core > > package. > > > > There are plenty of modules that they don't include, e.g. mod_fastcgi. > If you want that you download and build it against your installed > apache. Or you get mod_fcgid from extras. > > Your bolt-on client library would be in exactly the same boat as one of > those. I think Andrew Chernow is fundamentally confused about dynamic linking, which apache has to use because it doesn't know what type of file it has to handle, with linking, which is bound to the application code. pgtypes is bound to the application code so it is not like apache --- an application isn't going to be fed arbitrary pgtypes function calls it has to handle. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > > I think Andrew Chernow is fundamentally confused about dynamic linking, > which apache has to use because it doesn't know what type of file it has > to handle, with linking, which is bound to the application code. > pgtypes is bound to the application code so it is not like apache --- an > application isn't going to be fed arbitrary pgtypes function calls it > has to handle. > This discussion is now completely pointless as we have conceeded to a separate library. The community has spoken. We are trying to move on and open a discussion on the hook design. there are numbered questions: http://archives.postgresql.org/pgsql-hackers/2008-04/msg00565.php -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >> There is no need to pass hookData to the hook function. libpqtypes already >> accesses PGconn and PGresult directly so it can just access the hookData member. > > That's a habit you'd really be best advised to stop, if you're going to > be a separate library. Otherwise, any time we make an internal change > in the PGconn struct, it'll break your library --- and we have and will > feel free to do that without an external soname change. > > What parts of PGconn/PGresult do you need to touch that aren't exposed > already? > > regards, tom lane > Well, we manually create a result for arrays and composites. We also use pqResultAlloc in several places. I will have to look at the code again but I'm not sure we can be completely abstracted from the result struct. If you are suggesting that libpqtypes should not access internal libpq, than this idea won't work. We can pull all the code out and hook in, as you suggested, but we had no plans of abstracting from internal libpq. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: >> >> What parts of PGconn/PGresult do you need to touch that aren't exposed >> already? Don't need direct access to PGconn at all. result: null_field tupArrSize client_encoding (need a PGconn for this which might be dead) pqSetResultError pqResultAlloc pqResultStrdup Also, we basically need write access to every member inside a result object ... since we create our own for arrays and composites by copying the standard result members over. /* taken from dupresult inside handlers/utils.c */ PGresult *r = (PGresult *)malloc(sizeof(PGresult)); memset(r, 0, sizeof(PGresult)); /* copy some data from source result */ r->binary = args->get.result->binary; r->resultStatus = args->get.result->resultStatus; r->noticeHooks = args->get.result->noticeHooks; r->client_encoding = args->get.result->client_encoding; strcpy(r->cmdStatus, args->get.result->cmdStatus); [snip...] We can read args->get.result properties using PQfuncs with no problem. But we have no way of assign these values to our result 'r'. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Andrew Chernow wrote:
>> The core of what I am trying to ask is, there doesn't appear to be an
>> advantage to separating libpqtypes from libpq in terms of space.
> My guess is that if we provide an useful library, Redhat will distribute
> it some way or another.
The key phrase in that being "some way or another". Red Hat works with
a concept of core vs extras (or another way to look at it being what
comes on the CDs vs what you have to download from someplace). I think
it's highly likely that libpgtypes would end up in extras. If you do
not make it possible to package it that way (ie, separately from libpq),
it's more likely that it won't get packaged at all than that it will be
put in core.
regards, tom lane
On Wed, Apr 9, 2008 at 11:17 AM, Andrew Chernow <ac@esilo.com> wrote:
> We can read args->get.result properties using PQfuncs with no problem. But
> we have no way of assign these values to our result 'r'.
By the way, our decision to 'create the result' when exposing arrays
and composites saved us from creating lot of interface code to access
these structures from user code...the result already gave us what we
needed. Just in case anybody missed it, the way arrays of composites
would be handled in libpq would be like this:
PGarray array;
PQgetf(res, 0, "%foo[]", 0, &array); // foo being a composite(a int, b float)
PQclear(res);
for (i = 0; i < PQntuples(array.res); i++)
{ a int; b float; PQgetf(array.res, i, "%int %float", 0, &a, 1, &b); [...]
}
PQclear(array.res);
In the getf call, the brackets tell libpq that this is an array and a
new result is created to present the array...one column for simple
array, multiple columns if the array is composite. This can be
recursed if the composite is nested. We support all the PGget*
functions for the newly created array, providing the OIDs of the
internal composite elements for example. This is, IMO, payoff of
doing all the work with the type handlers...we don't have to make a
special version of getf that operates over arrays for example.
The upshot of this is that, however separation occurs, the PGresult is
a first class citizen to the types library.
merlin
Tom Lane wrote: > > The key phrase in that being "some way or another". Red Hat works with > a concept of core vs extras (or another way to look at it being what > comes on the CDs vs what you have to download from someplace). I think > it's highly likely that libpgtypes would end up in extras. If you do > not make it possible to package it that way (ie, separately from libpq), > it's more likely that it won't get packaged at all than that it will be > put in core. > > regards, tom lane > > I'll buy this. Better to be safe then sorry. This patch becomes more flexible and distributable when separated. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
> If you are suggesting that libpqtypes should not access internal libpq,
> than this idea won't work. We can pull all the code out and hook in, as
> you suggested, but we had no plans of abstracting from internal libpq.
That's exactly what I'm strongly suggesting. If you need to include
libpq-int.h at all, then your library will be forever fragile, and could
very well end up classified as "don't ship this at all, it's too likely
to break".
regards, tom lane
Tom Lane wrote: > > That's exactly what I'm strongly suggesting. If you need to include > libpq-int.h at all, then your library will be forever fragile, and could > very well end up classified as "don't ship this at all, it's too likely > to break". > > regards, tom lane > > I see your point, and it has a lot of merit. We am completely open to hearing how this can be solved. How do we duplicate a result object and customize many member values after the dup? Do we create a PGresultInfo struct containing all members of a result object that gets passed to "PGresuolt *PQresultDup(PGresult *source, PGresultInfo *info);"? Maybe it has a flags member that indicates which PQresultInfo members contain values that should override the source result. Any suggestions? This is where we are stumped. Everything else has several solutions. We are not debating this anymore, we are trying to implement it. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Tue, 2008-04-08 at 21:49 -0400, Merlin Moncure wrote:
> > * an "escapeIdent" function.
>
> not sure what this is...
>
Similar to the quote_ident() function in postgresql:
=> select quote_ident('foo"');quote_ident
-------------"foo"""
(1 row)
It could be called something like PQquoteIdent or PQescapeIdent.
Regards,Jeff Davis
Andrew Chernow <ac@esilo.com> writes:
>>> What parts of PGconn/PGresult do you need to touch that aren't exposed
>>> already?
> Don't need direct access to PGconn at all.
Oh, good, that makes things much easier.
> Also, we basically need write access to every member inside a result
> object ... since we create our own for arrays and composites by copying
> the standard result members over.
Hmm. I guess it wouldn't be completely out of the question to expose
the contents of PGresult as part of libpq's API. We haven't changed it
often, and it's hard to imagine a change that wouldn't be associated
with a major-version change anyhow. We could do some things to make it
a bit more bulletproof too, like change cmdStatus from fixed-size array
to pointer to allocated space so that CMDSTATUS_LEN doesn't become
part of the API.
Alternatively, we could keep it opaque and expose a bunch of manipulator
routines, but that might be a lot more cumbersome than it's worth.
regards, tom lane
Tom Lane wrote: > > Hmm. I guess it wouldn't be completely out of the question to expose > the contents of PGresult as part of libpq's API. We haven't changed it > often, and it's hard to imagine a change that wouldn't be associated > with a major-version change anyhow. We could do some things to make it > a bit more bulletproof too, like change cmdStatus from fixed-size array > to pointer to allocated space so that CMDSTATUS_LEN doesn't become > part of the API. > > Alternatively, we could keep it opaque and expose a bunch of manipulator > routines, but that might be a lot more cumbersome than it's worth. > > regards, tom lane > How about a proxy header (if such an animal exists). Maybe it is possible to take pg_result, and all structs it references, and put it in result-int.h. libpq-int.h would obviously include this. Also, any external library, like libpqtypes, that need direct access to a result can include result-int.h. This keeps pg_result and referenced structs out of libpq-fe.h. From a libpq user's viewpoint, nothing has changed. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote: > But I'll agree that cross-version hazards are a much more clear and > present danger. We've already broken binary compatibility at least > once since the current binary-I/O system was instituted (intervals > now have three fields not two) and there are obvious candidates for > future breakage, such as text locale/encoding support. But isn't that an argument *for* having support for the binary format in libpq in a form similar to what this patch offers? Then at least you'd be safe as long as your libpq-version is >= your server version. Currently, there seems to be no safe way to use the binary format, despite it's benefits for moving around large amounts of data. regards, Florian Pflug
Florian Pflug <fgp.phlo.org@gmail.com> writes:
> But isn't that an argument *for* having support for the binary format in
> libpq in a form similar to what this patch offers? Then at least you'd
> be safe as long as your libpq-version is >= your server version.
> Currently, there seems to be no safe way to use the binary format,
That's right, there isn't, and it's folly to imagine that anything like
this will make it "safe".
What we really put in the binary I/O support for was for COPY BINARY,
with a rather limited use-case of fast dump and reload between similar
server versions (you'll notice pg_dump does NOT use it). Yeah, we
expose it for clients to use, but they had better understand that they
are increasing their risk of cross-version portability problems.
regards, tom lane
Andrew Chernow <ac@esilo.com> writes:
> Tom Lane wrote:
>> Hmm. I guess it wouldn't be completely out of the question to expose
>> the contents of PGresult as part of libpq's API.
> How about a proxy header (if such an animal exists).
A separate header might be a good idea to discourage unnecessary
reliance on the struct, but it doesn't change the basic fact that
we'd be adding the struct to our ABI and couldn't change it without
a major library version number bump.
Perhaps we could do a partial exposure, where the exported struct
declaration contains "public" fields and there are some "private" ones
after that. This would still work for external creation of PGresults,
so long as all PGresults are initially manufactured by
PQmakeEmptyPGresult. That would give us a little bit of wiggle room
... in particular I'd be very tempted not to expose the fields
associated with space allocation (we could export pqResultAlloc
instead), nor anything not foreseeably needed by libpgtypes.
> Maybe it is
> possible to take pg_result, and all structs it references, and put it in
> result-int.h.
I'd think something like libpq-result.h would be a better choice of
name. The other seems likely to collide with who-knows-what from
other packages,
regards, tom lane
Tom Lane wrote: > > Perhaps we could do a partial exposure, where the exported struct > declaration contains "public" fields and there are some "private" ones > after that. > > I have another idea. It would remove a boat load of members that would need to be exposed (may remove them all). Can we make a variant of PQmakeEmptyPGresult? Maybe something like this: PGresult *PQdupPGresult( // maybe not the best name? PGconn *conn, PGresult *source, int numAttributes, int ntups); This starts off by calling PQmakeEmptyPGresult and then copying the below members from the provided 'source' result argument. - ExecStatusType resultStatus; - char cmdStatus[CMDSTATUS_LEN]; - int binary; - PGNoticeHooks noticeHooks; - int client_encoding; It would then preallocate attDescs and tuples which would also set numAttributes, ntups & tupArrSize. attdescs and tuples are not duplicated, only allocated based on the 'numAttributes' and 'ntups' arguments. Now libpqtypes only needs direct access to attDescs and tuples, for when it loops array elements or composite fields and copies stuff from the source result. Any interest in this direction? I am still thinking about how to abstract attDesc and tuples, I think it would require more API calls as well. curBlock, curOffset and spaceLeft were never copied (intialized to zero). We don't touch these. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote:
> Tom Lane wrote:
>>
>> Perhaps we could do a partial exposure, where the exported struct
>> declaration contains "public" fields and there are some "private" ones
>> after that.
>>
>
> I have another idea. It would remove a boat load of members that would
> need to be exposed (may remove them all).
>
> Can we make a variant of PQmakeEmptyPGresult? Maybe something like this:
>
>
Here is a quick implementation demonstrating the idea. It is very similar to
the patches internal dupresult function (handlers/utils.c).
/* numParameters, paramDescs, errFields, curBlock, curOffset and spaceLeft * are not assigned at all, initialized to
zero. errMsg is handled by * PQmakeEmptyPGresult. */
PGresult *PQdupPGresult( PGconn *conn, PGresult *source, int numAttributes, int ntups)
{ PGresult *r;
if(!source || numAttributes < 0 || ntups < 0) return NULL;
r = PQmakeEmptyPGresult(conn, source->resultStatus); if(!r) return NULL;
r->binary = source->binary; strcpy(r->cmdStatus, source->cmdStatus);
/* assigned by PQmakeEmptyPGresult when conn is not NULL */ if(!conn) { r->noticeHooks = source->noticeHooks;
r->client_encoding= source->client_encoding; }
r->attDescs = (PGresAttDesc *) pqResultAlloc(r, numAttributes * sizeof(PGresAttDesc), TRUE);
if(!r->attDescs) { PQclear(r); return NULL; }
r->numAttributes = numAttributes;
r->tuples = (PGresAttValue **) malloc(ntups * sizeof(PGresAttValue *));
if(!r->tuples) { PQclear(r); return NULL; }
r->ntups = ntups; r->tupArrSize = ntups;
return r;
}
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >>>> What parts of PGconn/PGresult do you need to touch that aren't exposed >>>> already? > >> Don't need direct access to PGconn at all. > > Oh, good, that makes things much easier. > Shoot! Feels like you always miss something. The patch uses PGconn's PQExpBuffer to set errors on a conn. Currently, there is no access to this buffer other than PQerrorMessage. Is the below okay? extern PQExpBuffer *PQgetErrorBuffer(PGconn *conn); // OR PQsetErrorMessage(conn, errstr) -- this felt strange to me The expbuffer API is public, so managing the returned PQExpBuffer would not require any additional API calls. While I am on the subject of things I missed, the patch also uses pqSetResultError (mostly during PQgetf). We no longer need direct access to anything inside pg_result. However, we would need 3 new API calls that would dup a result, set field descs and add tuples to a result. Below are prototypes of what I have so far (small footprint for all 3, maybe 100-150 lines). /* numParameters, paramDescs, errFields, curBlock, * curOffset and spaceLeft are not assigned at all, * initialized to zero. errMsg is handled by * PQmakeEmptyPGresult. attDescs and tuples are not * duplicated, only allocated based on 'ntups'and * 'numAttributes'. The idea is to dup the result * but customize attDescs and tuples. */ PGresult *PQresultDup( PGconn *conn, PGresult *source, int ntups, int numAttributes); /* Only for results returned by PQresultDup. This * will set the field descs for 'field_num'. The * PGresAttDesc structwas not used to avoid * exposing it. */ int PQresultSetFieldDesc( PGresult *res, int field_num, const char *name, Oid tableid, int columnid, int format, Oidtypid, int typlen, int typmod) /* Only for results returned by PQresultDup. This * will append a new tuple to res. A PGresAttValue * is allocated andput at index 'res->ntups'. This * is similar to pqAddTuple except that the tuples * table has been pre-allocated. Inour case, ntups * and numAttributes are known when calling resultDup. */ int PQresultAddTuple( PGresult *res, char *value, int len); The above would allow an external app to dup a result and customize its rows and columns. Our patch uses this for arrays and composites. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
"Andrew Chernow" <ac@esilo.com> writes: > Shoot! Feels like you always miss something. > > The patch uses PGconn's PQExpBuffer to set errors on a conn. Currently, there > is no access to this buffer other than PQerrorMessage. Is the below okay? Surely you would just provide a function to get pqtypes errors separate from libpq errors? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support!
Gregory Stark wrote:
> "Andrew Chernow" <ac@esilo.com> writes:
>
>> Shoot! Feels like you always miss something.
>>
>> The patch uses PGconn's PQExpBuffer to set errors on a conn. Currently, there
>> is no access to this buffer other than PQerrorMessage. Is the below okay?
>
> Surely you would just provide a function to get pqtypes errors separate from
> libpq errors?
>
That's extremely akward. Consider the below.
int getvalues(PGresult *res, int *x, char **t)
{ return PQgetf(res, "get the int and text);
}
if(getvalues(res, &x, &t)) printf("%s\n", PQresultErrorMessage(res));
How would the caller of getvalues know whether the error was generated
by a libpqtypes API call or by a libpq API call (like PQgetvalue)?
PQgetf should behave exactely as PQgetvalue does, in regards to errors.
Same holds true for PGconn. Normally, you are using PQputf which sets
the error in PQparamErrorMessage. Then there is PQparamCreate(conn) or
PQparamExec(conn, param, ...) and friends? PQparamExec calls
PQexecParams internally which can return NULL, setting an error message
inside PGconn. We felt our light-weight wrappers should be consistent
in behavior. If you get a NULL PGresult for a return value,
PQerrorMessage should be checked.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
"Andrew Chernow" <ac@esilo.com> writes: > How would the caller of getvalues know whether the error was generated by a > libpqtypes API call or by a libpq API call (like PQgetvalue)? PQgetf should > behave exactely as PQgetvalue does, in regards to errors. Hm. Well I was thinking of errors from database operations rather than things like PQgetvalue. I suppose we have to decide whether pqtypes is a "wrapper" around libpq in which case your functions would have to take the libpq error and copy it into your error state or an additional set of functions which are really part of appendage of libpq -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support!
On Thu, Apr 10, 2008 at 10:56 AM, Gregory Stark <stark@enterprisedb.com> wrote: > "Andrew Chernow" <ac@esilo.com> writes: > > How would the caller of getvalues know whether the error was generated by a > > libpqtypes API call or by a libpq API call (like PQgetvalue)? PQgetf should > > behave exactely as PQgetvalue does, in regards to errors. > > Hm. Well I was thinking of errors from database operations rather than things > like PQgetvalue. > > I suppose we have to decide whether pqtypes is a "wrapper" around libpq in > which case your functions would have to take the libpq error and copy it into > your error state or an additional set of functions which are really part of > appendage of libpq We see libpq types to be simply extending the api with more functions.We really only introduce new error handling with thePGparam struct that is following the same conventions that exist currently. The separation of libpqtypes out of libpq into a new library is an architectural change for organization purposes. We are not defining a new api or wrapping an existing one for new functionality (which is the same thing imo). With that in mind, libpq's error system is object based, not library based. Thee three objects that set errors are result, conn, (and now), param. In libpq terms there is no global error. (no errno, getlasterror, etc). So, if I understand you correctly, we see libpqtypes is an appendage in your terms...were you thinking along different lines? Consider that we don't reproduce all the things libpq offers, we share the same objects, etc. (In fact, we went though some acrobatics to introduce as little new behavior as possible). merlin
Andrew Chernow wrote:
>
> /* Only for results returned by PQresultDup. This
> * will append a new tuple to res. A PGresAttValue
> * is allocated and put at index 'res->ntups'. This
> * is similar to pqAddTuple except that the tuples
> * table has been pre-allocated. In our case, ntups
> * and numAttributes are known when calling resultDup.
> */
> int PQresultAddTuple(
> PGresult *res,
> char *value,
> int len);
>
PQresultAddTuple is not quite correct. The below is its replacement:
int PQresultSetFieldValue( PGresult *res, int tup_num, int field_num, char *value, int len)
Recap: PQresultDup, PQresultSetFieldDesc and PQresultSetFieldValue.
We feel this approach, which we initially thought wouldn't work, is
better than making pg_result public.
These functions could be useful outside of pqtypes, providing a way of
filtering/modifying a result object ... consider:
PGresult *execit(conn, stmt)
{ res = PQexec(conn, stmt); if(some_app_cond_is_true) { newres = PQresultDup(); // ... customize tups and
fields //PQresultSetFieldDesc and PQresultSetFieldValue PQclear(res); res = newres; } return res;
}
// res could be custom built
res = execit(conn, stmt);
PQclear(res);
This is not an everyday need, but I'm sure it could provide some niche
app functionality currently unavailable. Possibly useful to libpq
wrapper APIs. Either way, it works great for pqtypes and avoids
exposing pg_result.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
> Recap: PQresultDup, PQresultSetFieldDesc and PQresultSetFieldValue.
> We feel this approach, which we initially thought wouldn't work, is
> better than making pg_result public.
That doesn't seem to me to fit very well with libpq's internals ---
in particular the PQresult struct is not designed to support dynamically
adding columns, which retail PQresultSetFieldDesc() calls would seem to
require, and it's definitely not designed to allow that to happen after
you've begun to store data, which the apparent freedom to intermix
PQresultSetFieldDesc and PQresultSetFieldValue calls would seem to
imply.
Instead of PQresultSetFieldDesc, I think it might be better to provide a
call that creates an empty (of data) PGresult with a specified tuple
descriptor in one go.
regards, tom lane
Andrew Chernow <ac@esilo.com> writes:
> Gregory Stark wrote:
>> Surely you would just provide a function to get pqtypes errors separate from
>> libpq errors?
> That's extremely akward. Consider the below.
I'm just as suspicious of this as Greg is. In particular, I really
disagree with PQgetf storing an error message into a PGresult, because
that creates a semantic inconsistency. Up to now, PGresults have come
in two flavors, "okay" (which might hold data) and "error" (which hold
error messages). Now you're proposing to stick an error message into
an "okay" data-holding PGresult. Does that turn it into an "error"
result? Does its PQresultStatus change? Does it maybe forget the data
it used to hold?
An even more fundamental objection is that surely PQgetf's PGresult
argument should be const.
> int getvalues(PGresult *res, int *x, char **t)
> {
> return PQgetf(res, "get the int and text);
> }
> if(getvalues(res, &x, &t))
> printf("%s\n", PQresultErrorMessage(res));
This example is entirely unconvincing. There is no reason to be checking
PQresultErrorMessage at this point --- if you haven't already checked the
PGresult to be "okay", you should certainly not be trying to extract
fields from it. So I don't see that you are saving any steps here.
> PQgetf should behave exactely as PQgetvalue does, in regards to errors.
Uh-huh. You'll notice that PQgetvalue treats the PGresult as const.
> Same holds true for PGconn.
I'm not convinced about that side either. It doesn't fail the basic
const-ness test, perhaps, but it still looks mostly like you are trying
to avoid the necessity to think hard about how you're going to report
errors. You're going to have to confront the issue for operations that
only take a PGresult, and once you've got a good solution on that side
it might be better to use it for PQputf too.
regards, tom lane
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >> Recap: PQresultDup, PQresultSetFieldDesc and PQresultSetFieldValue. >> We feel this approach, which we initially thought wouldn't work, is >> better than making pg_result public. > > That doesn't seem to me to fit very well with libpq's internals --- > in particular the PQresult struct is not designed to support dynamically > adding columns, which retail PQresultSetFieldDesc() calls would seem to > require, and it's definitely not designed to allow that to happen after > you've begun to store data, which the apparent freedom to intermix > PQresultSetFieldDesc and PQresultSetFieldValue calls would seem to > imply. > > Instead of PQresultSetFieldDesc, I think it might be better to provide a > call that creates an empty (of data) PGresult with a specified tuple > descriptor in one go. > > regards, tom lane > > Are you against exposing PGresAttDesc? PGresult *PQresultDup( PGconn *conn, PGresult *res, int ntups, int numAttributes, PGresAttDesc *attDescs); If you don't want to expose PGresAttDesc, then the only other solution would be to pass parallel arrays of attname[], tableid[], columnid[], etc... Not the most friendly solution, but it would do the trick. Recap: Use new PQresultDup, throw out setfielddesc and keep setfieldvalue the same. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote:> PQresultErrorMessage at this point --- if you haven't already> checked the PGresult to be "okay", you shouldcertainly not be> trying to extract fields from it. So I don't see that you> are saving any steps here. Correct, I agree. Our thinking was flawed. The issue we face is that, unlike getvalue, getf can fail in many ways ... like bad format string or type mismatch. If you would rather us introduce a pqtypes specific error for getf. putf doesn't suffer from this issue because it uses PGparamErrorMessage. >> Same holds true for PGconn.>> I'm not convinced about that side either. It doesn't fail the basic> const-ness test, perhaps,but it still looks mostly like you>are trying to avoid the necessity to think hard about how you're>going to report The issue is not a matter of know-how, it is a matter of creating ambiguos situations in regards to where the error is (I'm thinking from a libpq user's perspective). This only applies to PQparamExec and fiends, NOT PQputf. All existing exec/send functions put an error in the conn (if the result is NULL check the conn). paramexec can fail prior to internally calling PQexecParams, in which case it returns NULL because no result has been constructed yet. The question is, where does the error go? res = paramexec(conn, param, ... if(!res) // check pgconn or pgparam? // can conn have an old error (false-pos) Not using the conn's error msg means that one must check the param and conn if the return value is NULL. I think the best behavior here is to check PQerrorMessage when any exec function returns a NULL result, including pqtypes. If not, I think it could get confusing to the API user. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
> PGresult *PQresultDup(
> PGconn *conn,
> PGresult *res,
> int ntups,
> int numAttributes,
> PGresAttDesc *attDescs);
I don't understand why this is a "dup" operation. How can you "dup"
if you are specifying a new tuple descriptor? I'd have expected
something like
PGresult *PQmakeResult(PGconn *conn, int numAttributes, PGresAttDesc *attDescs)
producing a zero-row PGRES_TUPLES_OK result that you can then load with
data via PQresultSetFieldValue calls. (Even the conn argument is a bit
of a wart, but I think we probably need it so we can copy some of its
private fields.)
Copying an existing PGresult might have some use too, but surely that
can't change its tuple descriptor.
regards, tom lane
Tom Lane wrote:
> Andrew Chernow <ac@esilo.com> writes:
>> PGresult *PQresultDup(
>> PGconn *conn,
>> PGresult *res,
>> int ntups,
>> int numAttributes,
>> PGresAttDesc *attDescs);
>
> I don't understand why this is a "dup" operation. How can you "dup"
> if you are specifying a new tuple descriptor? I'd have expected
> something like
>
> PGresult *PQmakeResult(PGconn *conn, int numAttributes, PGresAttDesc *attDescs)
>
> producing a zero-row PGRES_TUPLES_OK result that you can then load with
> data via PQresultSetFieldValue calls. (Even the conn argument is a bit
> of a wart, but I think we probably need it so we can copy some of its
> private fields.)
>
> Copying an existing PGresult might have some use too, but surely that
> can't change its tuple descriptor.
>
> regards, tom lane
>
Yeah, "dup" wasn't the best name.
You need more arguments to makeresult though, since you reomved the
'source' result. You need binary, cmdStatus, noticeHooks and
client_encoding.
PQmakeResult(conn, PQcmdStatus(res), PQbinaryTuples(res), ?client_encoding?, ?noticeHooks?, ntups, /* this
interactswith setfieldvalue */ numAttributes, attDescs);
For client_encoding and noticeHooks, the conn can be NULL. Previously,
we copied this info from the source result when conn was NULL.
> producing a zero-row PGRES_TUPLES_OK result that you can then>load with data via PQresultSetFieldValue calls.
I like this idea but you removed the 'ntups' argument. There is no way
to adjust ntups after the makeresult call, its a private member and
setfieldvalue has no concept of incrementing ntups. Since you are not
appending a tuple like pqAddTuple, or inserting one, you can't increment
ntups in setfieldvalue.
But, you could have a function like PQresultAddEmptyTuple(res) which
would have to be called before you can set field values on a tup_num.
The empty tuple would grow tuples when needed and increment ntups. The
new tuple would be zerod (all NULL values).
....something like below
res = PQmakeResult(...);
for(ntups)
{ PQresultAddEmptyTuple(res); // or PQresultMakeEmptyTuple? for(nfields) PQresultSetFieldValue(res, tup_num,
field_num,....);
}
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Andrew Chernow wrote:
>
> For client_encoding and noticeHooks, the conn can be NULL. Previously,
> we copied this info from the source result when conn was NULL.
>
>
Looks like we don't need client_encoding. My guess is, in our use case
noticeHooks can probably be NULL for the created result, when makeresult
is not provided a conn.
> ....something like below>> res = PQmakeResult(...);> for(ntups)> {> PQresultAddEmptyTuple(res); // or
PQresultMakeEmptyTuple?> for(nfields)> PQresultSetFieldValue(res, tup_num, field_num, ....);> }>>
I think a better idea would be to make setfieldvalue more dynamic,
removing the need for PQresultAddEmptyTuple. Only allow tup_num to be
<= res->ntups. This will only allow adding one tuple at a time. The
other option would allow arbitray tup_nums to be passed in, like ntups
is 1 but the provided tup_num is 67. In this case, we would have to
back fill. It seems better to only grow by one tuple at a time. We can
get it working either way, we just prefer one tuple at a time allocation.
int PQresultSetFieldValue( PGresult *res, int tup_num, int field_num, char *value, int len)
{ if(!check_field_value(res, field_num)) return FALSE;
if(tup_num > res->ntups) // error, tup_num must be <= res->ntups
if(res->ntups >= res->tupArrSize) // grow res->tuples
if(tup_num == res->ntups && !res->tuples[tup_num]) // need to allocate tuples[tup_num]
// blah ... blah ...
}
New PQmakeResult prototype:
PQmakeResult( PGconn *conn, const char *cmdStatus, int binaryTuples, int numAttributes, PGresAttDesc *attDescs);
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Andrew Chernow wrote: > res = paramexec(conn, param, ... > if(!res) > // check pgconn or pgparam? > // can conn have an old error (false-pos) > We will just always dump the error message into the param. If PQexecParams fails with a NULL result, we will copy PQerrorMessage over to param->errMsg. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/