Patch for DBD::Pg pg_relcheck problem

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 (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) ?

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, Pennsylvania 19073

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, Pennsylvania 19073

Perhaps compression should be added to the list of protocol changes.
This way, we can allow for per packet evaluation for compression.

--
Greg Copeland <greg@copelandconsulting.net>
Copeland Computer Consulting

Added to TODO.

---------------------------------------------------------------------------

Greg Copeland wrote:

Perhaps compression should be added to the list of protocol changes.
This way, we can allow for per packet evaluation for compression.

--
Greg Copeland <greg@copelandconsulting.net>
Copeland Computer Consulting

On Tue, 2002-12-10 at 21:50, Bruce Momjian wrote:

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, Pennsylvania 19073

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

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, Pennsylvania 19073

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, Pennsylvania 19073

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 data unless
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, Pennsylvania 19073

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, Pennsylvania 19073

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, Pennsylvania 19073