Обсуждение: Patch for DBD::Pg pg_relcheck problem
Attached is a patch against DBD::Pg 1.20 which fixes the driver-specific function 'table_attributes' for use with PostgreSQL 7.3 while maintaining backwards compatibility with older PostgreSQL versions. To avoid voodoo with PostgreSQL version numbers a check is made whether pg_relcheck exists and the appropriate query (either 7.3 or pre 7.3) executed. Tested with 7.3 and 7.1.3. This is hopefully a one off problem requiring this kind of check. I haven't had a chance to look at every function in Pg.pm, but it seems this is the only one "broken" by 7.3. Comments, corrections etc. welcome! Ian Barwick barwick@gmx.net
Ian Barwick <barwick@gmx.net> writes:
> To avoid voodoo with PostgreSQL version numbers
> a check is made whether pg_relcheck exists and
> the appropriate query (either 7.3 or pre 7.3)
> executed.
I would think that looking at version number (select version())
would be a much cleaner approach. Or do you think that direct
examination of pg_class is a version-independent operation?
This inquiry into pg_relcheck's existence is already arguably wrong
in 7.3 (since it's not taking account of which schema pg_relcheck
might be found in) and it can only go downhill in future versions.
regards, tom lane
On Monday 09 December 2002 17:03, Tom Lane wrote: > Ian Barwick <barwick@gmx.net> writes: > > To avoid voodoo with PostgreSQL version numbers > > a check is made whether pg_relcheck exists and > > the appropriate query (either 7.3 or pre 7.3) > > executed. > > I would think that looking at version number (select version()) > would be a much cleaner approach. Or do you think that direct > examination of pg_class is a version-independent operation? No, but I was hoping it will remain stable for long enough for what is basically a temporary work around until a revised version of DBD::Pg can be produced. It doesn't make any more assumptions about pg_class than are made elsewhere in the current Pg.pm. > This inquiry into pg_relcheck's existence is already arguably wrong > in 7.3 (since it's not taking account of which schema pg_relcheck > might be found in) and it can only go downhill in future versions. Doh. Knew I had to be missing something obvious. (Of course, anyone using current DBD::Pg with 7.3 as is will have to take extra care with system tables and schema namespaces anyway.) So out with the candle wax and pins ;-). Am I right in thinking that the string returned by SELECT version() starts with the word "PostgreSQL" followed by: a space; a single digit indicating the major version number; a full stop/ decimal point; a single digit indicating the minor version number; and either "interim release" number (e.g. ".1" in the case of 7.3.1), or "devel", "rc1" etc. ? And that this has been true since 6.x and will continue for the forseeable future (i.e. far far longer than the intended lifespan of attached patch)? Ian Barwick barwick@gmx.net Attached: revised patch
Ian Barwick <barwick@gmx.net> writes:
> So out with the candle wax and pins ;-). Am I right
> in thinking that the string returned by SELECT version()
> starts with the word "PostgreSQL" followed by:
> a space;
> a single digit indicating the major version number;
> a full stop / decimal point;
> a single digit indicating the minor version number;
> and either "interim release" number (e.g. ".1" in the case of 7.3.1), or
> "devel", "rc1" etc. ?
I would recommend parsing it the same way pg_dump does, see
_check_database_version(). Looks like it's basically skip-11-chars-
and-sscanf. Note the lack of assumptions about numbers of digits.
In the next protocol version update (hopefully 7.4) I would like to see
the basic version string (eg, "7.3.1" or "7.4devel") delivered to the
client automatically during connection startup and then available from a
libpq inquiry function. This would eliminate the need to call version()
explicitly and to know that you must skip "PostgreSQL " in its output.
However, it will only help for clients/libraries that are willing to
deal exclusively with 7.4-or-newer backends, so it will take a few
releases to become really useful.
regards, tom lane
(crossposting to hackers) On Tuesday 10 December 2002 00:47, Tom Lane wrote: > In the next protocol version update (hopefully 7.4) I would like to see > the basic version string (eg, "7.3.1" or "7.4devel") delivered to the > client automatically during connection startup and then available from a > libpq inquiry function. This would eliminate the need to call version() > explicitly and to know that you must skip "PostgreSQL " in its output. Something along the lines of char *PQversion(const PGconn *conn) ? > However, it will only help for clients/libraries that are willing to > deal exclusively with 7.4-or-newer backends, so it will take a few > releases to become really useful. Sounds good to me. Is it on the todo-list? (Couldn't see it there). Ian Barwick barwick@gmx.net
Ian Barwick <barwick@gmx.net> writes:
> Sounds good to me. Is it on the todo-list? (Couldn't see it there).
Probably not; Bruce for some reason has resisted listing protocol change
desires as an identifiable TODO category. There are a couple of threads
in the pghackers archives over the last year or so that discuss the
different things we want to do, though. (Improving the error-reporting
framework and fixing the COPY protocol are a couple of biggies I can
recall offhand.)
regards, tom lane
Ian Barwick writes:> On Tuesday 10 December 2002 00:47, Tom Lane wrote:> > In the next protocol version update (hopefully7.4) I would like to see> > the basic version string (eg, "7.3.1" or "7.4devel") delivered to the> > client automaticallyduring connection startup and then available from a> > libpq inquiry function. This would eliminate the needto call version()> > explicitly and to know that you must skip "PostgreSQL " in its output.> Something along the linesof > char *PQversion(const PGconn *conn) ? Probably: int PQversion(const PGconn *conn) would be better, and easier to parse? For example the value returned for 7.3.1 would be 7003001; for 7.4 7004000; for 101.10.2 101010002. This allows simple numerical tests... Lee.
(no followup to dbi-dev@perl.org, getting a little OT there) On Tuesday 10 December 2002 16:54, Lee Kindness wrote: > Ian Barwick writes: > > Something along the lines of > > char *PQversion(const PGconn *conn) ? > > Probably: > > int PQversion(const PGconn *conn) > > would be better, and easier to parse? For example the value returned > for 7.3.1 would be 7003001; for 7.4 7004000; for 101.10.2 > 101010002. This allows simple numerical tests... Sounds logical - I was evidently thinking in Perl ;-). For reference pg_dump currently parses the SELECT version() string into an integer thus: 7.2 70200 7.2.1 70201 7.3devel 70300 7.3rc1 70300 7.3.1 70301 7.3.99 70399 7.399.399 110299 101.10.2 1011002 (and just for fun: "11i Enterprise Edition with Bells and Whistles " returns -1 ;-) which works with minor release numbers of 99 and below. Ian Barwick barwick@gmx.net
Ian, can I please have a context diff, diff -c? --------------------------------------------------------------------------- Ian Barwick wrote: > On Monday 09 December 2002 17:03, Tom Lane wrote: > > Ian Barwick <barwick@gmx.net> writes: > > > To avoid voodoo with PostgreSQL version numbers > > > a check is made whether pg_relcheck exists and > > > the appropriate query (either 7.3 or pre 7.3) > > > executed. > > > > I would think that looking at version number (select version()) > > would be a much cleaner approach. Or do you think that direct > > examination of pg_class is a version-independent operation? > > No, but I was hoping it will remain stable for long enough > for what is basically a temporary work around until a revised version of > DBD::Pg can be produced. It doesn't make any more assumptions > about pg_class than are made elsewhere in the current Pg.pm. > > > This inquiry into pg_relcheck's existence is already arguably wrong > > in 7.3 (since it's not taking account of which schema pg_relcheck > > might be found in) and it can only go downhill in future versions. > > Doh. Knew I had to be missing something obvious. (Of course, > anyone using current DBD::Pg with 7.3 as is will have to take > extra care with system tables and schema namespaces anyway.) > > So out with the candle wax and pins ;-). Am I right > in thinking that the string returned by SELECT version() > starts with the word "PostgreSQL" followed by: > a space; > a single digit indicating the major version number; > a full stop / decimal point; > a single digit indicating the minor version number; > and either "interim release" number (e.g. ".1" in the case of 7.3.1), or > "devel", "rc1" etc. ? > And that this has been true since 6.x and will continue for the forseeable > future (i.e. far far longer than the intended lifespan of attached patch)? > > > Ian Barwick > barwick@gmx.net > > Attached: revised patch > > > > > > [ Attachment, skipping... ] -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Tom Lane wrote: > Ian Barwick <barwick@gmx.net> writes: > > Sounds good to me. Is it on the todo-list? (Couldn't see it there). > > Probably not; Bruce for some reason has resisted listing protocol change > desires as an identifiable TODO category. There are a couple of threads > in the pghackers archives over the last year or so that discuss the > different things we want to do, though. (Improving the error-reporting > framework and fixing the COPY protocol are a couple of biggies I can > recall offhand.) Listing protocol changes seemed too low-level for the TODO list, but I have kept the email messages. Today I updated the TODO list and added a section for them. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Thanks. Patch applied. David, time to package up a new version of DBD:Pg? --------------------------------------------------------------------------- Ian Barwick wrote: > On Monday 09 December 2002 17:03, Tom Lane wrote: > > Ian Barwick <barwick@gmx.net> writes: > > > To avoid voodoo with PostgreSQL version numbers > > > a check is made whether pg_relcheck exists and > > > the appropriate query (either 7.3 or pre 7.3) > > > executed. > > > > I would think that looking at version number (select version()) > > would be a much cleaner approach. Or do you think that direct > > examination of pg_class is a version-independent operation? > > No, but I was hoping it will remain stable for long enough > for what is basically a temporary work around until a revised version of > DBD::Pg can be produced. It doesn't make any more assumptions > about pg_class than are made elsewhere in the current Pg.pm. > > > This inquiry into pg_relcheck's existence is already arguably wrong > > in 7.3 (since it's not taking account of which schema pg_relcheck > > might be found in) and it can only go downhill in future versions. > > Doh. Knew I had to be missing something obvious. (Of course, > anyone using current DBD::Pg with 7.3 as is will have to take > extra care with system tables and schema namespaces anyway.) > > So out with the candle wax and pins ;-). Am I right > in thinking that the string returned by SELECT version() > starts with the word "PostgreSQL" followed by: > a space; > a single digit indicating the major version number; > a full stop / decimal point; > a single digit indicating the minor version number; > and either "interim release" number (e.g. ".1" in the case of 7.3.1), or > "devel", "rc1" etc. ? > And that this has been true since 6.x and will continue for the forseeable > future (i.e. far far longer than the intended lifespan of attached patch)? > > > Ian Barwick > barwick@gmx.net > > Attached: revised patch > > > > > > [ Attachment, skipping... ] -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Sunday 15 December 2002 00:37, Bruce Momjian wrote: > Thanks. Patch applied. David, time to package up a new version of DBD:Pg? Sorry Bruce, I've missed a couple of days through illness and am just catching up. I think I was gong to submit a slightly different version of the patch plus a couple of notes for the README file. Can you hold off 24 hours or so? Ian Barwick barwick@gmx.net
Ian Barwick wrote: > On Sunday 15 December 2002 00:37, Bruce Momjian wrote: > > Thanks. Patch applied. David, time to package up a new version of DBD:Pg? > > Sorry Bruce, I've missed a couple of days through illness and > am just catching up. I think I was gong to submit a slightly different > version of the patch plus a couple of notes for the README file. > Can you hold off 24 hours or so? Until we release it, the patch can just sit in CVS. Just send over a new version and I will back out the old one and put in the new one. I will not do any release until you are ready. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Saturday, December 14, 2002, at 03:37 PM, Bruce Momjian wrote: > Thanks. Patch applied. David, time to package up a new version of > DBD:Pg? After we figure out what to do with NULLs, I think. Best, David -- David Wheeler AIM: dwTheory david@wheeler.net ICQ: 15726394 http://david.wheeler.net/ Yahoo!: dew7e Jabber: Theory@jabber.org
David Wheeler wrote: > On Saturday, December 14, 2002, at 03:37 PM, Bruce Momjian wrote: > > > Thanks. Patch applied. David, time to package up a new version of > > DBD:Pg? > > After we figure out what to do with NULLs, I think. I thought you had that figured out, by flagging the param as bytea somehow? Are you thinking of something automatic? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Saturday, December 14, 2002, at 05:46 PM, Bruce Momjian wrote: > I thought you had that figured out, by flagging the param as bytea > somehow? Are you thinking of something automatic? No. Baldur Kristinsson points out that some people may pass a string with NUL characters to DBD::Pg to be inserted into something other than a bytea column. We have to figure out what to do when they do that. Baldur Kristinsson thinks we should just quietly strip them out (and throw a warning). Others think that it should throw an exception and croak. Tim said: The driver should always try to be as transparent as possible. The general principle is "don't mes with the users dataunless they've specifically asked you to (cf. ChopBlanks)". Since, however, nulls aren't really allowed in any PostgreSQL data type (except bytea, and then only if specifically bound as such to a prepared statement), I'm not sure what to do about this. We can't leave the data alone unless we just want PostgreSQL to throw an error (likely to be a mysterious error, as the user won't know why her data is getting truncated). I think...throw an exception, since PostgreSQL can't handle the null byte. Then it will be up to the user to clean up her data, and we won't have to touch it. Thoughts? David -- David Wheeler AIM: dwTheory david@wheeler.net ICQ: 15726394 http://david.wheeler.net/ Yahoo!: dew7e Jabber: Theory@jabber.org
David Wheeler wrote: > Since, however, nulls aren't really allowed in any PostgreSQL data type > (except bytea, and then only if specifically bound as such to a > prepared statement), I'm not sure what to do about this. We can't leave > the data alone unless we just want PostgreSQL to throw an error (likely > to be a mysterious error, as the user won't know why her data is > getting truncated). > > I think...throw an exception, since PostgreSQL can't handle the null > byte. Then it will be up to the user to clean up her data, and we won't > have to touch it. Yep, throw an error, and maybe point to bytea as the solution, until we have a better one. ;-) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Sunday 15 December 2002 02:09, Bruce Momjian wrote: > > Until we release it, the patch can just sit in CVS. Just send over a > new version and I will back out the old one and put in the new one. I > will not do any release until you are ready. Having looked it again I think it can stay as it is. (It is not very elegant but will fix things until something better comes along). Attached is a supplemental patch with an addition for the POD documentation in Pg.pm regarding the status of schema support in DBD::Pg. Ian Barwick barwick@gmx.net
Where are we on the release of a new DBDpg version? As I remember the only open item is handling binary values, but at this point, maybe we should just push out a release and fix it later. --------------------------------------------------------------------------- Bruce Momjian wrote: > > Thanks. Patch applied. David, time to package up a new version of DBD:Pg? > > --------------------------------------------------------------------------- > > Ian Barwick wrote: > > On Monday 09 December 2002 17:03, Tom Lane wrote: > > > Ian Barwick <barwick@gmx.net> writes: > > > > To avoid voodoo with PostgreSQL version numbers > > > > a check is made whether pg_relcheck exists and > > > > the appropriate query (either 7.3 or pre 7.3) > > > > executed. > > > > > > I would think that looking at version number (select version()) > > > would be a much cleaner approach. Or do you think that direct > > > examination of pg_class is a version-independent operation? > > > > No, but I was hoping it will remain stable for long enough > > for what is basically a temporary work around until a revised version of > > DBD::Pg can be produced. It doesn't make any more assumptions > > about pg_class than are made elsewhere in the current Pg.pm. > > > > > This inquiry into pg_relcheck's existence is already arguably wrong > > > in 7.3 (since it's not taking account of which schema pg_relcheck > > > might be found in) and it can only go downhill in future versions. > > > > Doh. Knew I had to be missing something obvious. (Of course, > > anyone using current DBD::Pg with 7.3 as is will have to take > > extra care with system tables and schema namespaces anyway.) > > > > So out with the candle wax and pins ;-). Am I right > > in thinking that the string returned by SELECT version() > > starts with the word "PostgreSQL" followed by: > > a space; > > a single digit indicating the major version number; > > a full stop / decimal point; > > a single digit indicating the minor version number; > > and either "interim release" number (e.g. ".1" in the case of 7.3.1), or > > "devel", "rc1" etc. ? > > And that this has been true since 6.x and will continue for the forseeable > > future (i.e. far far longer than the intended lifespan of attached patch)? > > > > > > Ian Barwick > > barwick@gmx.net > > > > Attached: revised patch > > > > > > > > > > > > > > [ Attachment, skipping... ] > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 359-1001 > + If your life is a hard drive, | 13 Roberts Road > + Christ can be your backup. | Newtown Square, Pennsylvania 19073 > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Patch applied. Thanks. --------------------------------------------------------------------------- Ian Barwick wrote: > On Sunday 15 December 2002 02:09, Bruce Momjian wrote: > > > > Until we release it, the patch can just sit in CVS. Just send over a > > new version and I will back out the old one and put in the new one. I > > will not do any release until you are ready. > > Having looked it again I think it can stay as it is. > (It is not very elegant but will fix things until something > better comes along). > > Attached is a supplemental patch with an addition for > the POD documentation in Pg.pm regarding the status > of schema support in DBD::Pg. > > > Ian Barwick > barwick@gmx.net [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073