libpq type system 0.9a

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.

The latest version of libpq type system is available here:
http://www.esilo.com/projects/postgresql/libpq/typesys-0.9a.tar.gz

The following modifications where made:
*) documentation fixes
*) parameter resets are no longer automatic
*) updated to patch vs. REL8_3_STABLE

Merlin Moncure & Andrew Chernow
eSilo

Merlin Moncure wrote:
> 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.
Sorry if this has been discussed before, but why is it necessary
to specify the type when calling PQgetf on a result? It seems that this
formatting string *always* has to match the type list of your select
statement, no?

regards, Florian Pflug

On Wed, Mar 5, 2008 at 5:47 PM, Florian G. Pflug <fgp(at)phlo(dot)org> wrote:
> Merlin Moncure wrote:
> > 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.
> Sorry if this has been discussed before, but why is it necessary
> to specify the type when calling PQgetf on a result? It seems that this
> formatting string *always* has to match the type list of your select
> statement, no?

yes...it always has to match. the format string requirements could in
theory be relaxed (for 'get') but this would break symmetry with 'put'
and you would lose a sanity check...getf like scanf writes directly
into application memory so the double-specifying (directly in the
format string and indirectly in the query) isn't necessarily a bad
thing. imagine if your application was 'select * from table' and one
of the field types changed...disaster.

merlin

Merlin Moncure wrote:
> On Wed, Mar 5, 2008 at 5:47 PM, Florian G. Pflug <fgp(at)phlo(dot)org> wrote:
>> Merlin Moncure wrote:
>> > 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.
>> Sorry if this has been discussed before, but why is it necessary
>> to specify the type when calling PQgetf on a result? It seems that this
>> formatting string *always* has to match the type list of your select
>> statement, no?
>
> yes...it always has to match. the format string requirements could in
> theory be relaxed (for 'get') but this would break symmetry with 'put'
> and you would lose a sanity check...getf like scanf writes directly
> into application memory so the double-specifying (directly in the
> format string and indirectly in the query) isn't necessarily a bad
> thing. imagine if your application was 'select * from table' and one
> of the field types changed...disaster.
>
> merlin
>
>

A few other reasons....

>>why is it necessary to specify the type when calling PQgetf on a result

Unlike PQgetvalue, all values returned by PQgetf are either native C types or
structures ... not C strings. When you call getf you must tell it what types to
read out of the result object. Like scanf, they must be the correctly sized
data types.

PGdate date;
int i4;
PQgetf(result, tup_num, "%date %int4", 0, &date, 1, &i4);

Specifying anything other than a %date or %int4 in the above example is a
programming error. You would be asking to fetch a value of the wrong type.
Without the formatting string, libpq would have to va_arg(ASSUME_T) your value.

// no specifier
int i;
PQgetf(result, tup, field, &i);

In the above, libpq would have to use PQftype to determine what the native C
type is of your variable argument. If PQftype returned INT8OID, you begin to
clobber your application's memory space ... va_arg(ap, long long) on a 32-bit
value. This problem is solved by telling libpq what data type you want from a
field.

Also, the libpq type system enforces strict type checking when performing getf
calls. This protects from mis-matches "programming errors" on types:

For example:

-- create table t (a int8);
PQresult *result = PQexec(conn, "SELECT a FROM t");
char *val = PQgetvalue(result, ...);
int a = atoi(val); // assumed its an int4

In the above example, the libpq user thinks the 'a' column of the 't' table is
an int4 when in fact its an int8. The above may work most of the time but will
eventually truncate the value and nip you in the butt. With PQgetf, you would
get an error saying the server returned an int8 and you are asking for an int4.
Thus, the programming bug would be squashed immediately.

Also, user-defined types are not known to libpq so PQftype would not really
work. They could if the libpq type system referenced data types by OID, but
this is not portable to other servers. It is more portable to use the type
name. For example, a company with 15 postgresql servers that use the same
collection of company-specific user-defined data types. The type names would be
the same across the 15 servers but there is no guarentee the OIDs would be.

Composites and arrays caused a few issues as well.

We also tried to provide as much protection as possible ... in the spirit of the
backend.

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

Merlin Moncure escribió:

> The latest version of libpq type system is available here:
> http://www.esilo.com/projects/postgresql/libpq/typesys-0.9a.tar.gz

This patch is not in diff -c format ... please provide a diff -c patch,
and add the URL to the wiki patch queue:
http://wiki.postgresql.org/wiki/CommitFest:March

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

On Tue, Mar 25, 2008 at 4:21 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Merlin Moncure escribió:
> > The latest version of libpq type system is available here:
> > http://www.esilo.com/projects/postgresql/libpq/typesys-0.9a.tar.gz
>
> This patch is not in diff -c format ... please provide a diff -c patch,
> and add the URL to the wiki patch queue:
> http://wiki.postgresql.org/wiki/CommitFest:March

converted to context diff (0.9b):
http://www.esilo.com/projects/postgresql/libpq/typesys-0.9b.tar.gz

will have wiki set up soon...need to get an account over there.

merlin

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.

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

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

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.

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

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/

On Fri, Apr 4, 2008 at 6:56 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> 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.

I think that is oversimplifying things a little bit. As Andrew stated
there are some aspects of the type system that would not so easily
abstracted out into a separate library. The type handlers them selves
could be moved out...but since they are basically hardcoded in the
server (for the built in types), why not do it in the client as well?
The libpq type system was deliberately designed so that user types
could be 'plugged in' .

I think a reasonable objective would be to organize the types a little
bit better in both the client and the server so there would be more
code reuse. We would support this change, but it would require some
changes to the server as well.

OTOH, we are proposing to extend the libpq interface. IMO, breaking
the libpq interface to separate libraries would only cause confusion.

merlin

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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-patches

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Dunstan <andrew(at)dunslane(dot)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(at)turnstep(dot)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(at)momjian(dot)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(at)turnstep(dot)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(at)turnstep(dot)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(at)gmail(dot)com> writes:
> On Tue, Apr 8, 2008 at 1:51 PM, Greg Sabino Mullane <greg(at)turnstep(dot)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(at)esilo(dot)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(at)dunslane(dot)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(at)commandprompt(dot)com> writes:
> On Tue, 08 Apr 2008 14:34:51 -0400
> Andrew Chernow <ac(at)esilo(dot)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(at)momjian(dot)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(at)commandprompt(dot)com> writes:
> > On Tue, 08 Apr 2008 14:34:51 -0400
> > Andrew Chernow <ac(at)esilo(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Chernow <ac(at)esilo(dot)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(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Chernow <ac(at)esilo(dot)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(at)svana(dot)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(at)momjian(dot)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(at)gmail(dot)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(at)esilo(dot)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(at)esilo(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)esilo(dot)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(at)esilo(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)turnstep(dot)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 from August, 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-dependent parts of
> ecpg out of the ecpg directory and this might be a solution, _even_ if
> it makes your library larger.
>

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(at)momjian(dot)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(at)postgresql(dot)org/msg102719.html

merlin

On Tue, Apr 8, 2008 at 9:28 PM, Jeff Davis <pgsql(at)j-davis(dot)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

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(at)j-davis(dot)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

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 <pgsql(at)j-davis(dot)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(at)sss(dot)pgh(dot)pa(dot)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 proposal is 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
// 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);
}

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 operators for 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 unknown or 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 string if 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(at)j-davis(dot)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(at)enterprisedb(dot)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(at)dunslane(dot)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 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.

--
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(at)dunslane(dot)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 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.

>>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(at)jabber(dot)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(at)esilo(dot)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(at)momjian(dot)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(at)esilo(dot)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(at)commandprompt(dot)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(at)esilo(dot)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(at)esilo(dot)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(at)esilo(dot)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(dot)phlo(dot)org(at)gmail(dot)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(at)esilo(dot)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(at)esilo(dot)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 struct was not used to avoid
* exposing it.
*/
int PQresultSetFieldDesc(
PGresult *res,
int field_num,
const char *name,
Oid tableid,
int columnid,
int format,
Oid typid,
int typlen,
int typmod)

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

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(at)esilo(dot)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(at)esilo(dot)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(at)esilo(dot)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(at)enterprisedb(dot)com> wrote:
> "Andrew Chernow" <ac(at)esilo(dot)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 the PGparam 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(at)esilo(dot)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(at)esilo(dot)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(at)esilo(dot)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 should certainly 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(at)esilo(dot)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(at)esilo(dot)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 interacts with 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.