Re: 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(at)hagander(dot)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(at)gmail(dot)com> wrote:
> On Tue, Dec 28, 2010 at 7:13 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)gmail(dot)com> writes:
> On Tue, Dec 28, 2010 at 7:13 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)hagander(dot)net> writes:
> On Tue, Dec 28, 2010 at 13:18, Robert Haas <robertmhaas(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Tue, Dec 28, 2010 at 13:18, Robert Haas <robertmhaas(at)gmail(dot)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(at)hagander(dot)net> writes:
> On Tue, Dec 28, 2010 at 16:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Tue, Dec 28, 2010 at 16:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)hagander(dot)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(at)gmail(dot)com> wrote:
> On Dec 29, 2010, at 10:14 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)hagander(dot)net> wrote:
> On Wed, Dec 29, 2010 at 19:49, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Dec 29, 2010, at 10:14 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)gmx(dot)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(at)hagander(dot)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(at)gmail(dot)com> writes:
> On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)gmail(dot)com> wrote:
> On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)2ndquadrant(dot)com> wrote:
> On Sun, Mar 27, 2011 at 4:09 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)hagander(dot)net> wrote:
> On Sun, Mar 27, 2011 at 04:02, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On Sun, Mar 27, 2011 at 4:09 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Mon, Jan 3, 2011 at 6:55 AM, Magnus Hagander <magnus(at)hagander(dot)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(at)hagander(dot)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(at)gmx(dot)net> writes:
> On sn, 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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> wrote:
> On Mon, Mar 28, 2011 at 7:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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/