Thread: [HACKERS] [Bug fix] PQsendQuery occurs error when target_session_attrs isset to read-write

Hello,

This this is my first posting to the mailing list.

I am interested in multiple hosts of libpq [1], then I found the bug in this feature. 
When I set "target_session_attrs" to "any" and call PQsendQuery, my application is succeeded.
However, when I set "target_session_attrs" to "read-write" and call PQsendQuery, "another command is already in
progress"is occurred. 
 
I attached the test application to reproduce this problem. 

I think this is because PQgetResult is not called until PQgetResult has returned a null pointer. 
So, I attached the patch for fix this. 

[1] https://www.postgresql.org/message-id/flat/20150818041850.GA5092@wagner.pp.ru#20150818041850.GA5092@wagner.pp.ru

Regards, 
Daisuke Higuchi


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 31, 2017 at 9:53 AM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:
> Hello,
>
> This this is my first posting to the mailing list.
>
> I am interested in multiple hosts of libpq [1], then I found the bug in this feature.
> When I set "target_session_attrs" to "any" and call PQsendQuery, my application is succeeded.
> However, when I set "target_session_attrs" to "read-write" and call PQsendQuery, "another command is already in
progress"is occurred.
 
> I attached the test application to reproduce this problem.
>
> I think this is because PQgetResult is not called until PQgetResult has returned a null pointer.
> So, I attached the patch for fix this.
>
> [1] https://www.postgresql.org/message-id/flat/20150818041850.GA5092@wagner.pp.ru#20150818041850.GA5092@wagner.pp.ru
>
Interestingly, when I don't set PGPORT, PGHOST I get the same error
with the C program. The patch fixes the problem.

Per the documentation [1], "PQgetResult must be called repeatedly
until it returns a null pointer, indicating that the command is
done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
case, violates that. The patch fixes it. The patch however jumps to
keep_going without changing conn->status, which means that it will end
up again in the same case. While that's fine, may be we should use a
local loop on PQgetResult() to keep the code readable.

I would have added a test with the patch, but it seems we don't have
much tests in interfaces/libpq. The bug is pretty trivial and would
have been caught easily had we had any tests.

[1] https://www.postgresql.org/docs/devel/static/libpq-async.html

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Please add this to the next commitfest, so that we don't forget it.

On Wed, Feb 1, 2017 at 3:58 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Tue, Jan 31, 2017 at 9:53 AM, Higuchi, Daisuke
> <higuchi.daisuke@jp.fujitsu.com> wrote:
>> Hello,
>>
>> This this is my first posting to the mailing list.
>>
>> I am interested in multiple hosts of libpq [1], then I found the bug in this feature.
>> When I set "target_session_attrs" to "any" and call PQsendQuery, my application is succeeded.
>> However, when I set "target_session_attrs" to "read-write" and call PQsendQuery, "another command is already in
progress"is occurred.
 
>> I attached the test application to reproduce this problem.
>>
>> I think this is because PQgetResult is not called until PQgetResult has returned a null pointer.
>> So, I attached the patch for fix this.
>>
>> [1]
https://www.postgresql.org/message-id/flat/20150818041850.GA5092@wagner.pp.ru#20150818041850.GA5092@wagner.pp.ru
>>
> Interestingly, when I don't set PGPORT, PGHOST I get the same error
> with the C program. The patch fixes the problem.
>
> Per the documentation [1], "PQgetResult must be called repeatedly
> until it returns a null pointer, indicating that the command is
> done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
> case, violates that. The patch fixes it. The patch however jumps to
> keep_going without changing conn->status, which means that it will end
> up again in the same case. While that's fine, may be we should use a
> local loop on PQgetResult() to keep the code readable.
>
> I would have added a test with the patch, but it seems we don't have
> much tests in interfaces/libpq. The bug is pretty trivial and would
> have been caught easily had we had any tests.
>
> [1] https://www.postgresql.org/docs/devel/static/libpq-async.html
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com] 
> Per the documentation [1], "PQgetResult must be called repeatedly
> until it returns a null pointer, indicating that the command is
> done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
> case, violates that. The patch fixes it. The patch however jumps to
> keep_going without changing conn->status, which means that it will end
> up again in the same case. While that's fine, may be we should use a
> local loop on PQgetResult() to keep the code readable.
Thank you for reviewing the patch. 
I created it with reference to pqSetenvPoll() in interfaces/libpq/fe-protocol2.c, 
but I certainly thought that readability is not good. 
I updated the patch, so I will add this to the next commitfest. 

Regards, 
Daisuke, Higuchi


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Feb 2, 2017 at 8:11 AM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:
> From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com]
>> Per the documentation [1], "PQgetResult must be called repeatedly
>> until it returns a null pointer, indicating that the command is
>> done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
>> case, violates that. The patch fixes it. The patch however jumps to
>> keep_going without changing conn->status, which means that it will end
>> up again in the same case. While that's fine, may be we should use a
>> local loop on PQgetResult() to keep the code readable.
> Thank you for reviewing the patch.
> I created it with reference to pqSetenvPoll() in interfaces/libpq/fe-protocol2.c,
> but I certainly thought that readability is not good.
> I updated the patch, so I will add this to the next commitfest.

Thanks for the patch.

The code expects that there will be two PQgetResult() calls required.
One to fetch the result of SHOW command and the other to extract NULL.
If we require more calls or unexpected results, we should throw and
error. The patch just checks the first result and consumes the
remaining without verifying them. Also, it looks like we can not clear
result of PQgetResult() before using the values or copying them
somewhere else [1]. Here's updated patch which tries to do that.
Please let me know if this looks good to you.


[1] https://www.postgresql.org/docs/devel/static/libpq-exec.html: PQgetvalue().
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Sorry, attached wrong patch. Here's the right one.

On Thu, Feb 2, 2017 at 10:04 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Thu, Feb 2, 2017 at 8:11 AM, Higuchi, Daisuke
> <higuchi.daisuke@jp.fujitsu.com> wrote:
>> From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com]
>>> Per the documentation [1], "PQgetResult must be called repeatedly
>>> until it returns a null pointer, indicating that the command is
>>> done.". The code in PQgetResult() under CONNECTION_CHECK_WRITABLE
>>> case, violates that. The patch fixes it. The patch however jumps to
>>> keep_going without changing conn->status, which means that it will end
>>> up again in the same case. While that's fine, may be we should use a
>>> local loop on PQgetResult() to keep the code readable.
>> Thank you for reviewing the patch.
>> I created it with reference to pqSetenvPoll() in interfaces/libpq/fe-protocol2.c,
>> but I certainly thought that readability is not good.
>> I updated the patch, so I will add this to the next commitfest.
>
> Thanks for the patch.
>
> The code expects that there will be two PQgetResult() calls required.
> One to fetch the result of SHOW command and the other to extract NULL.
> If we require more calls or unexpected results, we should throw and
> error. The patch just checks the first result and consumes the
> remaining without verifying them. Also, it looks like we can not clear
> result of PQgetResult() before using the values or copying them
> somewhere else [1]. Here's updated patch which tries to do that.
> Please let me know if this looks good to you.
>
>
> [1] https://www.postgresql.org/docs/devel/static/libpq-exec.html: PQgetvalue().
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Feb 2, 2017 at 1:34 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Thu, Feb 2, 2017 at 10:04 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> [1] https://www.postgresql.org/docs/devel/static/libpq-exec.html: PQgetvalue().
> The code expects that there will be two PQgetResult() calls required.
> One to fetch the result of SHOW command and the other to extract NULL.
> If we require more calls or unexpected results, we should throw and
> error. The patch just checks the first result and consumes the
> remaining without verifying them. Also, it looks like we can not clear
> result of PQgetResult() before using the values or copying them
> somewhere else [1]. Here's updated patch which tries to do that.
> Please let me know if this looks good to you.

This has not been added yet to the next CF. As Ashutosh mentioned
things tend to be easily forgotten. So I have added it here:
https://commitfest.postgresql.org/13/982/

I have noticed this typo on the way => s/requisted/requested/:
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2908,7 +2908,7 @@ keep_going:                       /* We will
come back to here until there is           }
           /*
-            * If a read-write connection is requisted check for same.
+            * If a read-write connection is requested check for same.            */           if
(conn->target_session_attrs!= NULL &&               strcmp(conn->target_session_attrs, "read-write") == 0)
 

Looking at the patch, I agree with Ashutosh. Any fix should be located
in the code path of CONNECTION_CHECK_WRITABLE which is the one
consuming the results.
-- 
Michael

From: Ashutosh Bapat [mailto:ashutosh.bapat@enterprisedb.com] 
>Sorry, attached wrong patch. Here's the right one.
> The code expects that there will be two PQgetResult() calls required.
> One to fetch the result of SHOW command and the other to extract NULL.
> If we require more calls or unexpected results, we should throw and
> error. The patch just checks the first result and consumes the
> remaining without verifying them. Also, it looks like we can not clear
> result of PQgetResult() before using the values or copying them
> somewhere else [1]. Here's updated patch which tries to do that. 
> Please let me know if this looks good to you.
Oh, I had a basic misunderstanding. Thank you for correct me. 
There is no problem in the patch you attached. I agree with you. 

Regards, 
Daisuke, Higuchi

From: Michael Paquier [mailto:michael.paquier@gmail.com] 
>This has not been added yet to the next CF. As Ashutosh mentioned
>things tend to be easily forgotten. So I have added it here:
>https://commitfest.postgresql.org/13/982/
Thank you for adding this problem to CF. 

> I have noticed this typo on the way => s/requisted/requested/:
>--- a/src/interfaces/libpq/fe-connect.c
>+++ b/src/interfaces/libpq/fe-connect.c
>@@ -2908,7 +2908,7 @@ keep_going:                       /* We will
>come back to here until there is
>            }
>            /*
>-            * If a read-write connection is requisted check for same.
>+            * If a read-write connection is requested check for same.
>             */
>            if (conn->target_session_attrs != NULL &&
>                strcmp(conn->target_session_attrs, "read-write") == 0)
I add this fix to the updated patch. 
This is based on the patch Ashutosh updated. 

Regards, 
Daisuke, Higuchi


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Feb 2, 2017 at 3:01 PM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:
> From: Michael Paquier [mailto:michael.paquier@gmail.com]
>>This has not been added yet to the next CF. As Ashutosh mentioned
>>things tend to be easily forgotten. So I have added it here:
>>https://commitfest.postgresql.org/13/982/
> Thank you for adding this problem to CF.

I have added this thread to the list of open items for PG10:
https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items
-- 
Michael

On Tue, Feb 14, 2017 at 12:11 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Feb 2, 2017 at 3:01 PM, Higuchi, Daisuke
> <higuchi.daisuke@jp.fujitsu.com> wrote:
>> From: Michael Paquier [mailto:michael.paquier@gmail.com]
>>>This has not been added yet to the next CF. As Ashutosh mentioned
>>>things tend to be easily forgotten. So I have added it here:
>>>https://commitfest.postgresql.org/13/982/
>> Thank you for adding this problem to CF.
>
> I have added this thread to the list of open items for PG10:
> https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items

Good catch, Michael.

I think the patch as presented probably isn't quite what we want,
because it waits synchronously for the second result to be ready.
Note that the wait for the first result is asynchronous: we check
PQisBusy and return without progressing the state machine until the
latter returns false; only then do we call PQgetResult().

But the typo fix is of course correct, and independent.  Committed that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Feb 15, 2017 at 11:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> I think the patch as presented probably isn't quite what we want,
> because it waits synchronously for the second result to be ready.
> Note that the wait for the first result is asynchronous: we check
> PQisBusy and return without progressing the state machine until the
> latter returns false; only then do we call PQgetResult().

OK, I have been poking at this problem. I think that we need to
introduce a new state in ConnStatusType telling "please consume all
results until PQgetResult returns NULL" which is CONNECTION_CONSUME in
the patch attached. And as long as all the results are not consumed,
the loop just keeps going. If more messages are being waited for with
PGisBusy returning true, then the loop requests for more data to read
by switching back to PGRES_POLLING_READING.

By the way, I am noticing as well that libpq.sgml is missing a
reference to CONNECTION_CHECK_WRITABLE. This should be present as
applications calling PQconnectPoll() could face such a status. I have
fixed this issue as well in the patch attached.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Feb 15, 2017 at 1:31 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 11:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I think the patch as presented probably isn't quite what we want,
>> because it waits synchronously for the second result to be ready.
>> Note that the wait for the first result is asynchronous: we check
>> PQisBusy and return without progressing the state machine until the
>> latter returns false; only then do we call PQgetResult().
>
> OK, I have been poking at this problem. I think that we need to
> introduce a new state in ConnStatusType telling "please consume all
> results until PQgetResult returns NULL" which is CONNECTION_CONSUME in
> the patch attached. And as long as all the results are not consumed,
> the loop just keeps going. If more messages are being waited for with
> PGisBusy returning true, then the loop requests for more data to read
> by switching back to PGRES_POLLING_READING.
>
> By the way, I am noticing as well that libpq.sgml is missing a
> reference to CONNECTION_CHECK_WRITABLE. This should be present as
> applications calling PQconnectPoll() could face such a status. I have
> fixed this issue as well in the patch attached.

Great, thanks!  This looks good to me, so committed.  Is there any
chance you can use your amazing TAP-test-creation powers to do some
automated testing of this feature?  For example, maybe we could at
least set up a master and a standby and somehow test that asking for a
read-only server picks the standby if it's first but asking for a
read-write server tries the standby and then switches to the master?
It would also be nice to test that probing a server that doesn't exist
fails, but I'm not sure how to create an IP/port combination that's
guaranteed to not work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Feb 16, 2017 at 1:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> Great, thanks!  This looks good to me, so committed.

Thanks.

> Is there any
> chance you can use your amazing TAP-test-creation powers to do some
> automated testing of this feature?  For example, maybe we could at
> least set up a master and a standby and somehow test that asking for a
> read-only server picks the standby if it's first but asking for a
> read-write server tries the standby and then switches to the master?
> It would also be nice to test that probing a server that doesn't exist
> fails, but I'm not sure how to create an IP/port combination that's
> guaranteed to not work.

It is possible to get a test easily in this area by abusing of the
fact that multiple -d switches defined in psql make it use only the
last value. By looking at psql() in PostgresNode.pm you would see what
I mean as -d is defined by default. Or we could just enforce
PGHOST/PGPORT as there is a method to get the unix domain directory.
Not sure which one of those two methods is most elegant though. I
would tend for just using PGHOST and PGPORT.
-- 
Michael

On Thu, Feb 16, 2017 at 10:55 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> It is possible to get a test easily in this area by abusing of the
> fact that multiple -d switches defined in psql make it use only the
> last value. By looking at psql() in PostgresNode.pm you would see what
> I mean as -d is defined by default. Or we could just enforce
> PGHOST/PGPORT as there is a method to get the unix domain directory.
> Not sure which one of those two methods is most elegant though. I
> would tend for just using PGHOST and PGPORT.

What do you think about the patch attached then? As env{PGHOST} is set
once and for all in INIT for each test run, I have gone with the
approach of building connection strings and feed that to psql -d. I
have reused 001_stream_rep.pl so as connections are done on already
existing nodes, making the test really cheap. Here is how the tests
look:
# testing connection parameter target_session_attrs
ok 5 - connect to node master if mode "read-write" and master,standby_1 listed
ok 6 - connect to node master if mode "read-write" and standby_1,master listed
ok 7 - connect to node master if mode "any" and master,standby_1 listed
ok 8 - connect to node standby_1 if mode "any" and standby_1,master listed

I have registered an entry in the CF here:
https://commitfest.postgresql.org/13/1014/
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Feb 20, 2017 at 11:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 10:55 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> It is possible to get a test easily in this area by abusing of the
>> fact that multiple -d switches defined in psql make it use only the
>> last value. By looking at psql() in PostgresNode.pm you would see what
>> I mean as -d is defined by default. Or we could just enforce
>> PGHOST/PGPORT as there is a method to get the unix domain directory.
>> Not sure which one of those two methods is most elegant though. I
>> would tend for just using PGHOST and PGPORT.
>
> What do you think about the patch attached then? As env{PGHOST} is set
> once and for all in INIT for each test run, I have gone with the
> approach of building connection strings and feed that to psql -d. I
> have reused 001_stream_rep.pl so as connections are done on already
> existing nodes, making the test really cheap. Here is how the tests
> look:
> # testing connection parameter target_session_attrs
> ok 5 - connect to node master if mode "read-write" and master,standby_1 listed
> ok 6 - connect to node master if mode "read-write" and standby_1,master listed
> ok 7 - connect to node master if mode "any" and master,standby_1 listed
> ok 8 - connect to node standby_1 if mode "any" and standby_1,master listed
>
> I have registered an entry in the CF here:
> https://commitfest.postgresql.org/13/1014/

Thanks!  Committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Feb 27, 2017 at 3:12 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> Thanks!  Committed.

Thanks.
-- 
Michael