libpq type system 0.9a

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

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:

I know there has been lots of versions and technical feedback related to
this proposed feature. However, I have talked to Tom and neither of us
see sufficient user request for this capability to add this code into
the core server. I recommend you place it on pgfoundry and see if you
can get a sufficient user-base there. If you need something exposed by
libpq that is not there already, please let us know.

One interesting idea would be if ecpg could use this functionality in
place of its own type-specific functions --- if that were the case, we
could reconsider adding this to core because it would be required by
ecpg.

Sorry for the bad news. I think we all hoped that enough interest would
be generated for this to be accepted.

I think you should conduct a wider survey before you make that decision.
In particular, I'd like to hear from driver writers like Greg Sabino
Mullane and Jeff Davis, as well as regular libpq users.

cheers

andrew

Andrew Dunstan wrote:

Bruce Momjian wrote:

I know there has been lots of versions and technical feedback related to
this proposed feature. However, I have talked to Tom and neither of us
see sufficient user request for this capability to add this code into
the core server. I recommend you place it on pgfoundry and see if you
can get a sufficient user-base there. If you need something exposed by
libpq that is not there already, please let us know.

One interesting idea would be if ecpg could use this functionality in
place of its own type-specific functions --- if that were the case, we
could reconsider adding this to core because it would be required by
ecpg.

Sorry for the bad news. I think we all hoped that enough interest would
be generated for this to be accepted.

I think you should conduct a wider survey before you make that decision.
In particular, I'd like to hear from driver writers like Greg Sabino
Mullane and Jeff Davis, as well as regular libpq users.

Well, they are welcome to chime in but the patch has been out the for a
while and they haven't spoken yet. We have to base our decisions on
people who do chime in.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Andrew Dunstan <andrew@dunslane.net> writes:

I think you should conduct a wider survey before you make that decision.
In particular, I'd like to hear from driver writers like Greg Sabino
Mullane and Jeff Davis, as well as regular libpq users.

Well, the survey's already been taken, pretty much: there's been just
about no positive feedback in all the time that this proposal's been
discussed. I haven't noticed anyone except Andrew and Merlin saying
that they wanted this or would use it.

Currently there's about 400K of C source code in libpq. The patch as-is
adds more than 100K, and it would surely get larger if we actively
pursued this line of development. (For one thing, the density of
comments in the submitted patch is well below what I'd find acceptable;
by the time it was commented to a level comparable to the existing libpq
code, it'd be a lot more than 100K.) That's a pretty big increment for
a facility that it appears would be used only by a small minority of
users. I think that at minimum we'd have to insist on it being
refactored as a separate library, and then the case for it being in core
rather than on pgfoundry seems kinda weak.

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

I think you should conduct a wider survey before you make that decision.
In particular, I'd like to hear from driver writers like Greg Sabino
Mullane and Jeff Davis, as well as regular libpq users.

I can state that there would be almost zero chance this would ever be
used by DBD::Pg, as it would seem to add overhead with no additional
functionality over what we already have. Unless I'm misreading what it
does and someone can make the case why I should use it.

- --
Greg Sabino Mullane greg@turnstep.com
PGP Key: 0x14964AC8 200804081349
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkf7sGkACgkQvJuQZxSWSsi8FgCgkGUGh2ieOAtvNlXX6orjO8oc
0bIAoPF7ojxM1c38kw7+L4Ar7FRZmdrn
=U2BM
-----END PGP SIGNATURE-----

On Tue, Apr 8, 2008 at 12:59 PM, Bruce Momjian <bruce@momjian.us> wrote:

Sorry for the bad news. I think we all hoped that enough interest would
be generated for this to be accepted.

I think that's really unfortunate. Personally, I think that anyone
who did any amount of C coding against libpq at all would never have
any reason to code in the traditional fashion (PQexec, etc). Anyone
who would claim otherwise IMO does not code vs. libpq or does not
completely understand what we are trying to do.

In particular, I think that the decision to so quickly shut the door
on the ability to support arrays and composites in binary on the
client side. Contrary to what is stated there have been considerable
requests for this on the various lists.

I am dismayed that throughout this process there has been no
substantive discussion (save for Tom) on what we were trying to do,
only to be informed about rejection based on an internal discussion.
What issues were raised were opaque and sans reasoning or
justification of how that would improve the patch or the functionality
(move to separate library for example -- how would this improve
things?). Our follow ups were not followed up. We would have been
delighted to take suggestions.

I attributed the silence to general lack of interest and anticipated
this response. However I think that those involved should step back
and take a look at what they are walking away from here.

merlin

On Tue, Apr 8, 2008 at 1:51 PM, Greg Sabino Mullane <greg@turnstep.com> wrote:

I think you should conduct a wider survey before you make that decision.
In particular, I'd like to hear from driver writers like Greg Sabino
Mullane and Jeff Davis, as well as regular libpq users.

I can state that there would be almost zero chance this would ever be
used by DBD::Pg, as it would seem to add overhead with no additional
functionality over what we already have. Unless I'm misreading what it
does and someone can make the case why I should use it.

does DBD::pg move arrays in and out of the server? do you parse them
yourself? if you answer yes to either question you should take a
second look.

merlin

Merlin Moncure escribi�:

I attributed the silence to general lack of interest and anticipated
this response. However I think that those involved should step back
and take a look at what they are walking away from here.

I suggest you take a survey on a more widely read forum, like
pgsql-general or even pgsql-announce. Keep in mind that many of the
most active people in pgsql-hackers is not actually writing libpq
programs (I know I am not.)

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

Greg Sabino Mullane wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

I think you should conduct a wider survey before you make that decision.
In particular, I'd like to hear from driver writers like Greg Sabino
Mullane and Jeff Davis, as well as regular libpq users.

I can state that there would be almost zero chance this would ever be
used by DBD::Pg, as it would seem to add overhead with no additional
functionality over what we already have. Unless I'm misreading what it
does and someone can make the case why I should use it.

- --
Greg Sabino Mullane greg@turnstep.com
PGP Key: 0x14964AC8 200804081349
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkf7sGkACgkQvJuQZxSWSsi8FgCgkGUGh2ieOAtvNlXX6orjO8oc
0bIAoPF7ojxM1c38kw7+L4Ar7FRZmdrn
=U2BM
-----END PGP SIGNATURE-----

This idea is for the libpq user, although driver writers could find it
handy as well. Really, anyone who uses libpq directly. That's the real
audience.

I don't know what overhead greg is referring to. How is DBD::pg
handling arrays of composites? Are you parsing text output? Wouldn't
it be less overhead to avoid text parsing and transmit binary data?

no additional functionality over what we already have

What about user-defined type registration, sub-classing user or built-in
type handlers, handling of all data types in binary. There is a lot
of new functionality added by this patch to the existing libpq.

I don't think the appropriate audience got a look at this, maybe posting
on general or libpq lists. From my perspective as a long time C coder,
this made my application code cleaner, easier to maintain and faster in
many cases. It removed a lot of code that is now handled by this patch.

I am not sure why Tom is worried about source code size, normally the
concern is linked size. Code comments were never finished, as the
library was changing so much to meet some requests. Instead, we focused
on providing API documentation and the overall idea (over 1000 lines).
This changed much less than the implementation.

I think the real issue is simply the wrong audience. Its the coder in
the field making heavy use of libpq that would find this appealing, not
really backend hackers.

It is disappointing because I was excited to here ideas from others,
which never happened.

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