Thread: Libpq PGRES_COPY_BOTH - version compatibility
Part of this may be my C skills not being good enough - if so, please enlighten me :-) My pg_streamrecv no longer works with 9.1, because it returns PGRES_COPY_BOTH instead of PGRES_COPY_OUT when initating a copy. That's fine. So I'd like to make it work on both. Specifically, I would like it to check for PGRES_COPY_BOTH if the server is 9.1 and PGRES_COPY_OUT if it's 9.0. Which can be done by checking the server version. However, when built against a libpq 9.0, it doesn't even have the symbol PGRES_COPY_BOTH. And I can't check for the presence of said symbol using #ifdef, since it's an enum. Nor is there a #define available to check the version of the header. Is there any way to check this at compile time (so I know if I can use the symbol or not), without using autoconf (I don't want to bring in such a huge dependency for a tiny program)? Also, I notice that PGRES_COPY_BOTH was inserted "in the middle" of the enum. Doesn't that mean we can get incorrect values for e.g. PGRES_FATAL_ERROR if the client is built against one version of libpq but executes against another? Shouldn't all such enum values always be added at the end? Finaly, as long as I only use "the 9.0 style replication", PGRES_COPY_BOTH is actually unnecessary, right? It will work exactly the same way as PGRES_COPY_OUT? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Tue, Dec 28, 2010 at 7:13 AM, Magnus Hagander <magnus@hagander.net> wrote: > Part of this may be my C skills not being good enough - if so, please > enlighten me :-) > > My pg_streamrecv no longer works with 9.1, because it returns > PGRES_COPY_BOTH instead of PGRES_COPY_OUT when initating a copy. > That's fine. > > So I'd like to make it work on both. Specifically, I would like it to > check for PGRES_COPY_BOTH if the server is 9.1 and PGRES_COPY_OUT if > it's 9.0. Which can be done by checking the server version. > > However, when built against a libpq 9.0, it doesn't even have the > symbol PGRES_COPY_BOTH. And I can't check for the presence of said > symbol using #ifdef, since it's an enum. Nor is there a #define > available to check the version of the header. > > Is there any way to check this at compile time (so I know if I can use > the symbol or not), without using autoconf (I don't want to bring in > such a huge dependency for a tiny program)? Adding a #define to our headers that you can test for seems like the way to go. > Also, I notice that PGRES_COPY_BOTH was inserted "in the middle" of > the enum. Doesn't that mean we can get incorrect values for e.g. > PGRES_FATAL_ERROR if the client is built against one version of libpq > but executes against another? Shouldn't all such enum values always be > added at the end? I think you are right, and that we should fix this. > Finaly, as long as I only use "the 9.0 style replication", > PGRES_COPY_BOTH is actually unnecessary, right? It will work exactly > the same way as PGRES_COPY_OUT? So far, the protocol message is all we've changed. I keep hoping some update synchronous replication patches are going to show up, but so far they haven't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Dec 28, 2010 at 13:18, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Dec 28, 2010 at 7:13 AM, Magnus Hagander <magnus@hagander.net> wrote: >> Part of this may be my C skills not being good enough - if so, please >> enlighten me :-) >> >> My pg_streamrecv no longer works with 9.1, because it returns >> PGRES_COPY_BOTH instead of PGRES_COPY_OUT when initating a copy. >> That's fine. >> >> So I'd like to make it work on both. Specifically, I would like it to >> check for PGRES_COPY_BOTH if the server is 9.1 and PGRES_COPY_OUT if >> it's 9.0. Which can be done by checking the server version. >> >> However, when built against a libpq 9.0, it doesn't even have the >> symbol PGRES_COPY_BOTH. And I can't check for the presence of said >> symbol using #ifdef, since it's an enum. Nor is there a #define >> available to check the version of the header. >> >> Is there any way to check this at compile time (so I know if I can use >> the symbol or not), without using autoconf (I don't want to bring in >> such a huge dependency for a tiny program)? > > Adding a #define to our headers that you can test for seems like the way to go. That's kind of what I was going for ;) Since it's libpq-fe.h, I think it would have to be another one of those edited by src/tools/version_stamp.pl that Tom doesn't like ;) I don't see another way though - since we don't pull the configure output files into libpq-fe.h (and shouldn't). >> Also, I notice that PGRES_COPY_BOTH was inserted "in the middle" of >> the enum. Doesn't that mean we can get incorrect values for e.g. >> PGRES_FATAL_ERROR if the client is built against one version of libpq >> but executes against another? Shouldn't all such enum values always be >> added at the end? > > I think you are right, and that we should fix this. Phew, at least I'm not completely lost :-) Will you take care of it? >> Finaly, as long as I only use "the 9.0 style replication", >> PGRES_COPY_BOTH is actually unnecessary, right? It will work exactly >> the same way as PGRES_COPY_OUT? > > So far, the protocol message is all we've changed. I keep hoping some > update synchronous replication patches are going to show up, but so > far they haven't. Well, assuming I run it in async mode, would the protocol be likely to change even then? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Dec 28, 2010 at 7:13 AM, Magnus Hagander <magnus@hagander.net> wrote: >> Also, I notice that PGRES_COPY_BOTH was inserted "in the middle" of >> the enum. Doesn't that mean we can get incorrect values for e.g. >> PGRES_FATAL_ERROR if the client is built against one version of libpq >> but executes against another? Shouldn't all such enum values always be >> added at the end? > I think you are right, and that we should fix this. Yes, that was a completely wrong move :-( regards, tom lane
Magnus Hagander <magnus@hagander.net> writes: > On Tue, Dec 28, 2010 at 13:18, Robert Haas <robertmhaas@gmail.com> wrote: >> Adding a #define to our headers that you can test for seems like the way to go. > That's kind of what I was going for ;) I don't see the point. You're going to need a *run time* test on PQserverVersion to figure out what the server will return, no? Also, if you really do need to figure out which PG headers you're compiling against, looking at catversion.h is the accepted way to do it. There's no need for yet another symbol. regards, tom lane
On Tue, Dec 28, 2010 at 16:15, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Tue, Dec 28, 2010 at 13:18, Robert Haas <robertmhaas@gmail.com> wrote: >>> Adding a #define to our headers that you can test for seems like the way to go. > >> That's kind of what I was going for ;) > > I don't see the point. You're going to need a *run time* test on > PQserverVersion to figure out what the server will return, no? I need *both*. > Also, if you really do need to figure out which PG headers you're > compiling against, looking at catversion.h is the accepted way to do it. > There's no need for yet another symbol. This file is, AFAIK, not included with client installs? It's definitely not present in the libpq-dev package on debian. It's a backend development file, no? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes: > On Tue, Dec 28, 2010 at 16:15, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Also, if you really do need to figure out which PG headers you're >> compiling against, looking at catversion.h is the accepted way to do it. >> There's no need for yet another symbol. > This file is, AFAIK, not included with client installs? It's > definitely not present in the libpq-dev package on debian. It's a > backend development file, no? [ shrug... ] We can't be held responsible for stupid packaging decisions by distros. regards, tom lane
On Wed, Dec 29, 2010 at 16:12, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Tue, Dec 28, 2010 at 16:15, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Also, if you really do need to figure out which PG headers you're >>> compiling against, looking at catversion.h is the accepted way to do it. >>> There's no need for yet another symbol. > >> This file is, AFAIK, not included with client installs? It's >> definitely not present in the libpq-dev package on debian. It's a >> backend development file, no? > > [ shrug... ] We can't be held responsible for stupid packaging > decisions by distros. Running "make install" in src/interfaces/libpq does not install catversion.h. If it's required to know which version of the libpq headers are in use, it should be, shouldn't it? We can be held responsible for the packaging decisions if they use *our* "make install" commands, imho. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Dec 29, 2010, at 10:14 AM, Magnus Hagander <magnus@hagander.net> wrote: > We can be held responsible for the packaging decisions if they use > *our* "make install" commands, imho. Yep. ...Robert
On Wed, Dec 29, 2010 at 19:49, Robert Haas <robertmhaas@gmail.com> wrote: > On Dec 29, 2010, at 10:14 AM, Magnus Hagander <magnus@hagander.net> wrote: >> We can be held responsible for the packaging decisions if they use >> *our* "make install" commands, imho. > > Yep. So, as I see it there are two ways of doing it - install a catversion.h file and include it from libpq-fe.h, or modify the libpq-fe.h. I still think modifying libpq-fe.h is the better of these choices - but either of them would work. But is the catversion value really the best interface for the user? This is about libpq functionality level, which really has nothing to do with the backend catalog, does it? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Sun, Jan 2, 2011 at 4:36 AM, Magnus Hagander <magnus@hagander.net> wrote: > On Wed, Dec 29, 2010 at 19:49, Robert Haas <robertmhaas@gmail.com> wrote: >> On Dec 29, 2010, at 10:14 AM, Magnus Hagander <magnus@hagander.net> wrote: >>> We can be held responsible for the packaging decisions if they use >>> *our* "make install" commands, imho. >> >> Yep. > > So, as I see it there are two ways of doing it - install a > catversion.h file and include it from libpq-fe.h, or modify the > libpq-fe.h. I still think modifying libpq-fe.h is the better of these > choices - but either of them would work. But is the catversion value > really the best interface for the user? This is about libpq > functionality level, which really has nothing to do with the backend > catalog, does it? It doesn't seem to me that a change of this type requires a catversion bump. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On tis, 2010-12-28 at 13:13 +0100, Magnus Hagander wrote: > My pg_streamrecv no longer works with 9.1, because it returns > PGRES_COPY_BOTH instead of PGRES_COPY_OUT when initating a copy. > That's fine. > > So I'd like to make it work on both. Specifically, I would like it to > check for PGRES_COPY_BOTH if the server is 9.1 and PGRES_COPY_OUT if > it's 9.0. Which can be done by checking the server version. ISTM that the correct fix is to increment to protocol version number to 3.1 and send PGRES_COPY_OUT if the client requests version 3.0. That's what the version numbers are for, no?
On Sun, Jan 2, 2011 at 15:07, Peter Eisentraut <peter_e@gmx.net> wrote: > On tis, 2010-12-28 at 13:13 +0100, Magnus Hagander wrote: >> My pg_streamrecv no longer works with 9.1, because it returns >> PGRES_COPY_BOTH instead of PGRES_COPY_OUT when initating a copy. >> That's fine. >> >> So I'd like to make it work on both. Specifically, I would like it to >> check for PGRES_COPY_BOTH if the server is 9.1 and PGRES_COPY_OUT if >> it's 9.0. Which can be done by checking the server version. > > ISTM that the correct fix is to increment to protocol version number to > 3.1 and send PGRES_COPY_OUT if the client requests version 3.0. That's > what the version numbers are for, no? In a way - yes. I assume we didn't do that because it's considered "internal". It still won't help in my situation though - I need to know what version of the libpq headers I have in order to even be able to *compile* the program. At runtime, I could check against the server version, and get around it. That said, if we are going to incorporate pg_streamrecv in the backend for 9.1, *my* problem goes away, as does the problem directly related to the change of PGRES_COPY_OUT. But the basic principle of the problem still remains... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus@hagander.net> wrote: >> ISTM that the correct fix is to increment to protocol version number to >> 3.1 and send PGRES_COPY_OUT if the client requests version 3.0. That's >> what the version numbers are for, no? > > In a way - yes. I assume we didn't do that because it's considered "internal". > > It still won't help in my situation though - I need to know what > version of the libpq headers I have in order to even be able to > *compile* the program. At runtime, I could check against the server > version, and get around it. This is listed on the open items list as "raise protocol version number", but the above discussion suggests both that this might be unnecessary and that it might not solve Magnus's problem anyway. What do we want to do here? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus@hagander.net> wrote: >>> ISTM that the correct fix is to increment to protocol version number to >>> 3.1 and send PGRES_COPY_OUT if the client requests version 3.0. �That's >>> what the version numbers are for, no? >> It still won't help in my situation though - I need to know what >> version of the libpq headers I have in order to even be able to >> *compile* the program. At runtime, I could check against the server >> version, and get around it. > This is listed on the open items list as "raise protocol version > number", but the above discussion suggests both that this might be > unnecessary and that it might not solve Magnus's problem anyway. > What do we want to do here? I'm not in favor of bumping the protocol version number for this. Magnus is correct that that'd be the right thing to do in an abstract sense; but we haven't bumped the protocol version number since 7.4, and so I have no faith that clients will behave sensibly. I think we'd be much more likely to break things than to accomplish anything useful. Given the small fraction of client programs that will care, and the fact that a protocol bump doesn't fix the whole issue anyway, working around it on the client side seems much the best compromise. regards, tom lane
On Sun, Mar 27, 2011 at 4:09 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus@hagander.net> wrote: >>> ISTM that the correct fix is to increment to protocol version number to >>> 3.1 and send PGRES_COPY_OUT if the client requests version 3.0. That's >>> what the version numbers are for, no? >> >> In a way - yes. I assume we didn't do that because it's considered "internal". >> >> It still won't help in my situation though - I need to know what >> version of the libpq headers I have in order to even be able to >> *compile* the program. At runtime, I could check against the server >> version, and get around it. > > This is listed on the open items list as "raise protocol version > number", but the above discussion suggests both that this might be > unnecessary and that it might not solve Magnus's problem anyway. > > What do we want to do here? We add an option as to how the protocol behaves, with default as 3.0. Older clients will not know about the new option and so will not request it. Magnus gets his new functionality, nothing breaks. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Sun, Mar 27, 2011 at 04:02, Simon Riggs <simon@2ndquadrant.com> wrote: > On Sun, Mar 27, 2011 at 4:09 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus@hagander.net> wrote: >>>> ISTM that the correct fix is to increment to protocol version number to >>>> 3.1 and send PGRES_COPY_OUT if the client requests version 3.0. That's >>>> what the version numbers are for, no? >>> >>> In a way - yes. I assume we didn't do that because it's considered "internal". >>> >>> It still won't help in my situation though - I need to know what >>> version of the libpq headers I have in order to even be able to >>> *compile* the program. At runtime, I could check against the server >>> version, and get around it. >> >> This is listed on the open items list as "raise protocol version >> number", but the above discussion suggests both that this might be >> unnecessary and that it might not solve Magnus's problem anyway. >> >> What do we want to do here? > > We add an option as to how the protocol behaves, with default as 3.0. > Older clients will not know about the new option and so will not > request it. > > Magnus gets his new functionality, nothing breaks. No he doesn't. Not yet - it needs the version check that's added to 9.1 - but it would have been needed for 9.0. So in a similar situation at the next release it would be fixed, but not here. That doesn't mean we shouldn't do this (haven't reconsidered the whole thread) - but it doesn't solve the issue I originally raised. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Sun, Mar 27, 2011 at 9:09 PM, Magnus Hagander <magnus@hagander.net> wrote: > On Sun, Mar 27, 2011 at 04:02, Simon Riggs <simon@2ndquadrant.com> wrote: >> On Sun, Mar 27, 2011 at 4:09 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus@hagander.net> wrote: >>>>> ISTM that the correct fix is to increment to protocol version number to >>>>> 3.1 and send PGRES_COPY_OUT if the client requests version 3.0. That's >>>>> what the version numbers are for, no? >>>> >>>> In a way - yes. I assume we didn't do that because it's considered "internal". >>>> >>>> It still won't help in my situation though - I need to know what >>>> version of the libpq headers I have in order to even be able to >>>> *compile* the program. At runtime, I could check against the server >>>> version, and get around it. >>> >>> This is listed on the open items list as "raise protocol version >>> number", but the above discussion suggests both that this might be >>> unnecessary and that it might not solve Magnus's problem anyway. >>> >>> What do we want to do here? >> >> We add an option as to how the protocol behaves, with default as 3.0. >> Older clients will not know about the new option and so will not >> request it. >> >> Magnus gets his new functionality, nothing breaks. > > No he doesn't. Not yet - it needs the version check that's added to > 9.1 - but it would have been needed for 9.0. So in a similar situation > at the next release it would be fixed, but not here. > > That doesn't mean we shouldn't do this (haven't reconsidered the whole > thread) - but it doesn't solve the issue I originally raised. Test the release number? >= 9.0 What's wrong with that? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mar 27, 2011, at 4:09 PM, Magnus Hagander <magnus@hagander.net> wrote: > That doesn't mean we shouldn't do this (haven't reconsidered the whole > thread) - but it doesn't solve the issue I originally raised. I'm somewhat inclined to just remove this from the list of open items. It doesn't seem clear what the action item is here,or even that there is one, so holding up beta for it doesn't seem right. When/if we figure out what needs to be done,we can revisit the issue. Anyone disagree? ...Robert
On sön, 2011-03-27 at 00:20 -0400, Tom Lane wrote: > but we haven't bumped the protocol version number since 7.4, > and so I have no faith that clients will behave sensibly So we will never change the minor protocol version, because we've never done it and don't know whether it works? Perhaps the answer then is to do it more often?
Peter Eisentraut <peter_e@gmx.net> writes: > On sön, 2011-03-27 at 00:20 -0400, Tom Lane wrote: >> but we haven't bumped the protocol version number since 7.4, >> and so I have no faith that clients will behave sensibly > So we will never change the minor protocol version, because we've never > done it and don't know whether it works? My feeling is we should leave it for a time when we have a protocol change to make that's actually of interest to clients (and, therefore, some benefit to them in return for any possible breakage). The case for doing it to benefit only walsender/walreceiver seems vanishingly thin to me, because in practice those are going to be quite useless if you don't have the same PG version installed at both ends anyway. Now if we had a track record showing that we could tweak the protocol version without causing problems, it'd be fine with me to do it for this usage. But we don't, and this particular case doesn't seem like the place to start. regards, tom lane
I wrote: > Now if we had a track record showing that we could tweak the protocol > version without causing problems, it'd be fine with me to do it for this > usage. But we don't, and this particular case doesn't seem like the > place to start. And, btw, a moment's study of the protocol version checking code in postmaster.c shows that bumping the minor version number to 3.1 *would* break things: a client requesting 3.1 from a current postmaster would get a failure. Maybe we oughta change that logic --- it's not clear to me that there's any meaningful difference between major and minor numbers given the current postmaster behavior. regards, tom lane
On Mon, Mar 28, 2011 at 7:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> Now if we had a track record showing that we could tweak the protocol >> version without causing problems, it'd be fine with me to do it for this >> usage. But we don't, and this particular case doesn't seem like the >> place to start. > > And, btw, a moment's study of the protocol version checking code in > postmaster.c shows that bumping the minor version number to 3.1 *would* > break things: a client requesting 3.1 from a current postmaster would > get a failure. Given that, it seems that there is far more downside than upside to this particular change, and we shouldn't do it. Accordingly, I'm going to mark the open item "raise protocol version number" closed. > Maybe we oughta change that logic --- it's not clear to me that there's > any meaningful difference between major and minor numbers given the > current postmaster behavior. I don't think this would be a bad thing to do if we're fairly clear that it's correct and won't break anything, but I don't think it's worth delaying beta for, so I propose not to add it to the open items list unless someone else feels otherwise. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Mar 31, 2011 at 17:35, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Mar 28, 2011 at 7:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I wrote: >>> Now if we had a track record showing that we could tweak the protocol >>> version without causing problems, it'd be fine with me to do it for this >>> usage. But we don't, and this particular case doesn't seem like the >>> place to start. >> >> And, btw, a moment's study of the protocol version checking code in >> postmaster.c shows that bumping the minor version number to 3.1 *would* >> break things: a client requesting 3.1 from a current postmaster would >> get a failure. > > Given that, it seems that there is far more downside than upside to > this particular change, and we shouldn't do it. Accordingly, I'm > going to mark the open item "raise protocol version number" closed. +1. >> Maybe we oughta change that logic --- it's not clear to me that there's >> any meaningful difference between major and minor numbers given the >> current postmaster behavior. > > I don't think this would be a bad thing to do if we're fairly clear > that it's correct and won't break anything, but I don't think it's > worth delaying beta for, so I propose not to add it to the open items > list unless someone else feels otherwise. Perhaps this part should go on the TODO list then? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/