statement_timeout is not working as expected with postgres_fdw

On Thu, Apr 20, 2017 at 4:08 PM, Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:

Hello All,

The query on foreign table hangs due to network down of remote server for
near about 16 minutes before exiting.
statement_timeout is expected to work in this case as well but when i tried
statement_timeout, it is not working as expected.

Below is test case to reproduce the issue: [I am testing this on two
different systems]

1: Set the postgres_fdw

postgres=# CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(host '172.16.173.237', dbname 'postgres', port '5432',keepalives '1',
keepalives_interval '3',keepalives_idle '3', keepalives_count '1');
CREATE SERVER
postgres=# CREATE USER MAPPING FOR postgres SERVER myserver OPTIONS (user
'postgres', password 'edb');
CREATE USER MAPPING
postgres=# CREATE FOREIGN TABLE test(id int) SERVER myserver;
CREATE FOREIGN TABLE
postgres=# select * from test;
id
----
1
10
2
1
(4 rows)

postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+------+---------------+----------
public | test | foreign table | postgres
(1 row)
postgres=#
postgres=# select * from pg_foreign_server ;
srvname | srvowner | srvfdw | srvtype | srvversion | srvacl |
srvoptions

----------+----------+--------+---------+------------+--------+------------------------------------------------------------------------------------------------------------------
-------
myserver | 10 | 16387 | | | |
{host=172.16.173.237,dbname=postgres,port=5432,keepalives=1,keepalives_interval=3,keepalives_idle=3,keepalives_co
unt=1}
(1 row)

3. Execute the insert command.
E.g: insert into test values (generate_series(1,1000000));
* You need to do network down of remote server during insert command.

postgres=# set statement_timeout to 6000;
SET
postgres=# insert into test values (generate_series(1,1000000));
WARNING: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

CONTEXT: Remote SQL command: ABORT TRANSACTION
ERROR: could not receive data from server: No route to host

CONTEXT: Remote SQL command: INSERT INTO public.test(id) VALUES ($1)
Time: 931427.002 ms

It was in hang state for approx 15-16 minute before exit.

In pg_log, i see below error for above query:
=========
2017-04-20 15:22:02 IST ERROR: could not receive data from server: No route
to host
2017-04-20 15:22:02 IST CONTEXT: Remote SQL command: INSERT INTO
public.test(id) VALUES ($1)
2017-04-20 15:22:02 IST STATEMENT: insert into test values
(generate_series(1,1000000));
2017-04-20 15:22:02 IST WARNING: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
2017-04-20 15:22:02 IST CONTEXT: Remote SQL command: ABORT TRANSACTION
==========

I tested the patch submitted by Ashutosh Bapat on community
[/messages/by-id/CAFjFpRdcWw4h0a-zrL-EiaekkPj8O0GR2M1FwZ1useSRfRm3-g@mail.gmail.com%5D
to make the statement_timeout working and i can see statement_timeout is
working but it is taking some extra time as statement_timeout.

Thanks for testing the patch.

postgres=# set statement_timeout to 6000;
SET
postgres=#
postgres=# \timing on
Timing is on.
postgres=# insert into test values (generate_series(1,1000000));
-- [after executing query immediately disconnect the ethernet on remote
server ]
2017-04-20 07:10:51.588 IST [10467] ERROR: canceling statement due to
statement timeout
2017-04-20 07:10:51.588 IST [10467] STATEMENT: insert into test values
(generate_series(1,1000000));
2017-04-20 07:11:25.590 IST [10467] WARNING: discarding connection 0xfe4260
because of failed cancel request: PQcancel() -- connect() failed: No route
to host
WARNING: discarding connection 0xfe4260 because of failed cancel request:
PQcancel() -- connect() failed: No route to host

ERROR: canceling statement due to statement timeout
Time: 40001.765 ms (00:40.002)
postgres=#
postgres=#

In above case, I got the error related to statement timeout after 6 seconds,
but it it taking more time (approx 34 sec and it is varing each time if you
see below) to terminate the connection and to exit from query. As per the
tcp keepavlies settings for foreign server, it should take max 6 sec to
terminate the connection.

The logs above show that 34 seconds elapsed between starting to abort
the transaction and knowing that the foreign server is unreachable. It
looks like it took that much time for the local server to realise that
the foreign server is not reachable. Looking at PQcancel code, it
seems to be trying to connect to the foreign server to cancel the
query. But somehow it doesn't seem to honor connect_timeout setting.
Is that expected?

Irrespective of what PQcancel does, it looks like postgres_fdw should
just slam the connection if query is being aborted because of
statement_timeout. But then pgfdw_xact_callback() doesn't seem to have
a way to know whether this ABORT is because of user's request to
cancel the query, statement timeout, an abort because of some other
error or a user requested abort. Except statement timeout (may be
user's request to cancel the query?), it should try to keep the
connection around to avoid any future reconnection. But I am not able
to see how can we provide that information to pgfdw_xact_callback().

--
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

Hello,

At Thu, 20 Apr 2017 19:57:30 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in <CAFjFpRdMD25Lia_J0P1eiKzNXAXtH9AGNx2vCZ2dBPNffNOaog@mail.gmail.com>

On Thu, Apr 20, 2017 at 4:08 PM, Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:

Hello All,

The query on foreign table hangs due to network down of remote server for
near about 16 minutes before exiting.
statement_timeout is expected to work in this case as well but when i tried
statement_timeout, it is not working as expected.

Below is test case to reproduce the issue: [I am testing this on two
different systems]

1: Set the postgres_fdw

postgres=# CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(host '172.16.173.237', dbname 'postgres', port '5432',keepalives '1',
keepalives_interval '3',keepalives_idle '3', keepalives_count '1');
CREATE SERVER
postgres=# CREATE USER MAPPING FOR postgres SERVER myserver OPTIONS (user
'postgres', password 'edb');
CREATE USER MAPPING
postgres=# CREATE FOREIGN TABLE test(id int) SERVER myserver;
CREATE FOREIGN TABLE
postgres=# select * from test;
id
----
1
10
2
1
(4 rows)

postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+------+---------------+----------
public | test | foreign table | postgres
(1 row)
postgres=#
postgres=# select * from pg_foreign_server ;
srvname | srvowner | srvfdw | srvtype | srvversion | srvacl |
srvoptions

----------+----------+--------+---------+------------+--------+------------------------------------------------------------------------------------------------------------------
-------
myserver | 10 | 16387 | | | |
{host=172.16.173.237,dbname=postgres,port=5432,keepalives=1,keepalives_interval=3,keepalives_idle=3,keepalives_co
unt=1}
(1 row)

3. Execute the insert command.
E.g: insert into test values (generate_series(1,1000000));
* You need to do network down of remote server during insert command.

postgres=# set statement_timeout to 6000;
SET
postgres=# insert into test values (generate_series(1,1000000));
WARNING: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

CONTEXT: Remote SQL command: ABORT TRANSACTION
ERROR: could not receive data from server: No route to host

CONTEXT: Remote SQL command: INSERT INTO public.test(id) VALUES ($1)
Time: 931427.002 ms

It was in hang state for approx 15-16 minute before exit.

In pg_log, i see below error for above query:
=========
2017-04-20 15:22:02 IST ERROR: could not receive data from server: No route
to host
2017-04-20 15:22:02 IST CONTEXT: Remote SQL command: INSERT INTO
public.test(id) VALUES ($1)
2017-04-20 15:22:02 IST STATEMENT: insert into test values
(generate_series(1,1000000));
2017-04-20 15:22:02 IST WARNING: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
2017-04-20 15:22:02 IST CONTEXT: Remote SQL command: ABORT TRANSACTION
==========

I tested the patch submitted by Ashutosh Bapat on community
[/messages/by-id/CAFjFpRdcWw4h0a-zrL-EiaekkPj8O0GR2M1FwZ1useSRfRm3-g@mail.gmail.com%5D
to make the statement_timeout working and i can see statement_timeout is
working but it is taking some extra time as statement_timeout.

Thanks for testing the patch.

postgres=# set statement_timeout to 6000;
SET
postgres=#
postgres=# \timing on
Timing is on.
postgres=# insert into test values (generate_series(1,1000000));
-- [after executing query immediately disconnect the ethernet on remote
server ]
2017-04-20 07:10:51.588 IST [10467] ERROR: canceling statement due to
statement timeout
2017-04-20 07:10:51.588 IST [10467] STATEMENT: insert into test values
(generate_series(1,1000000));
2017-04-20 07:11:25.590 IST [10467] WARNING: discarding connection 0xfe4260
because of failed cancel request: PQcancel() -- connect() failed: No route
to host
WARNING: discarding connection 0xfe4260 because of failed cancel request:
PQcancel() -- connect() failed: No route to host

ERROR: canceling statement due to statement timeout
Time: 40001.765 ms (00:40.002)
postgres=#
postgres=#

In above case, I got the error related to statement timeout after 6 seconds,
but it it taking more time (approx 34 sec and it is varing each time if you
see below) to terminate the connection and to exit from query. As per the
tcp keepavlies settings for foreign server, it should take max 6 sec to
terminate the connection.

The logs above show that 34 seconds elapsed between starting to abort
the transaction and knowing that the foreign server is unreachable. It
looks like it took that much time for the local server to realise that
the foreign server is not reachable. Looking at PQcancel code, it
seems to be trying to connect to the foreign server to cancel the
query. But somehow it doesn't seem to honor connect_timeout setting.
Is that expected?

Yes, and No. I think PQcancel requires connection timeout, but I
think it is not necessariry the same with that of a foreign
server.

Irrespective of what PQcancel does, it looks like postgres_fdw should
just slam the connection if query is being aborted because of
statement_timeout. But then pgfdw_xact_callback() doesn't seem to have
a way to know whether this ABORT is because of user's request to
cancel the query, statement timeout, an abort because of some other
error or a user requested abort. Except statement timeout (may be
user's request to cancel the query?), it should try to keep the
connection around to avoid any future reconnection. But I am not able
to see how can we provide that information to pgfdw_xact_callback().

Expiration of statement_timeout doesn't mean a stall of foreign
connections. If we are to keep connections by, for example, a
cancel request from client, we also should keep them on
statememt_timeout because it is not necessariry triggered by a
stall of foreign connection.

I think we can detect a stall of the channel where the foreign
connections are on by a cancel request with a very short timeout,
although it is a bit incorrect.

I reconsider this problem and my proposal for this issue is as
the follows.

- Foreign servers have a new options 'stall_detection_threshold'
in milliseconds, maybe defaults to connect_timeout of the
foreign server setting. For many foreign servers in a local
network, it could be lowered to several tens of milliseconds.

-+ Invoke PQcancel with the stall_detection_threashold. (It
 | doesn't accept such parameter so far)
 |
 +- If PQcancel returns with timeout, slam the connection down.
 |
 +- Else we continue to send ABORT TRANSACTION to the connection.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
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, Apr 25, 2017 at 1:31 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

The logs above show that 34 seconds elapsed between starting to abort
the transaction and knowing that the foreign server is unreachable. It
looks like it took that much time for the local server to realise that
the foreign server is not reachable. Looking at PQcancel code, it
seems to be trying to connect to the foreign server to cancel the
query. But somehow it doesn't seem to honor connect_timeout setting.
Is that expected?

Yes, and No. I think PQcancel requires connection timeout, but I
think it is not necessariry the same with that of a foreign
server.

Since connect_timeout is property of foreign server, it should be
honored by any connection made to that server from local server
including the one by PQcancel().

Irrespective of what PQcancel does, it looks like postgres_fdw should
just slam the connection if query is being aborted because of
statement_timeout. But then pgfdw_xact_callback() doesn't seem to have
a way to know whether this ABORT is because of user's request to
cancel the query, statement timeout, an abort because of some other
error or a user requested abort. Except statement timeout (may be
user's request to cancel the query?), it should try to keep the
connection around to avoid any future reconnection. But I am not able
to see how can we provide that information to pgfdw_xact_callback().

Expiration of statement_timeout doesn't mean a stall of foreign
connections. If we are to keep connections by, for example, a
cancel request from client, we also should keep them on
statememt_timeout because it is not necessariry triggered by a
stall of foreign connection.

When statement_timeout completes, we don't want to spend more time in
trying to cancel queries: esp when there are many foreign server, each
consuming some "timeout" time OR even trying to send Abort transaction
statement. Instead, we should slam those down. I consider this to be
different from query cancellation since query cancellation doesn't
have a hard bound on time, although we would like to cancel the
running query as fast as possible. Rethinking about it, probably we
should slam down the connection in case of query cancel as well.

I think we can detect a stall of the channel where the foreign
connections are on by a cancel request with a very short timeout,
although it is a bit incorrect.

I reconsider this problem and my proposal for this issue is as
the follows.

- Foreign servers have a new options 'stall_detection_threshold'
in milliseconds, maybe defaults to connect_timeout of the
foreign server setting. For many foreign servers in a local
network, it could be lowered to several tens of milliseconds.

A connect_timeout less than 2 seconds is not encouraged.
https://www.postgresql.org/docs/devel/static/libpq-connect.html. So,
we can not set stall_detection_threshold to be smaller than 2 seconds.
statement_timeout however is set in milliseconds, so 2 seconds per
connection would be quite a lot compared to statement_timeout setting.
Waiting to cancel query for 2 seconds when the statement_timeout
itself is 2 seconds would mean the query would be cancelled after 4
seconds, which is kind of funny.

--
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

At Tue, 25 Apr 2017 15:43:59 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in <CAFjFpRe6uf3-nU-orTEAJ0_CKb3MMiPQ5AHJ_SwDguV7Sjs6Ww@mail.gmail.com>

On Tue, Apr 25, 2017 at 1:31 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

The logs above show that 34 seconds elapsed between starting to abort
the transaction and knowing that the foreign server is unreachable. It
looks like it took that much time for the local server to realise that
the foreign server is not reachable. Looking at PQcancel code, it
seems to be trying to connect to the foreign server to cancel the
query. But somehow it doesn't seem to honor connect_timeout setting.
Is that expected?

Yes, and No. I think PQcancel requires connection timeout, but I
think it is not necessariry the same with that of a foreign
server.

Since connect_timeout is property of foreign server, it should be
honored by any connection made to that server from local server
including the one by PQcancel().

The two connections have different characteristics. A query
(client) connection should be finally established. On the other
we may give up to establish a cancel connection in certain cases.
For example, when a network stall is anticipated, we can rather
choose to trash the session on it than waiting for the timeout.

Irrespective of what PQcancel does, it looks like postgres_fdw should
just slam the connection if query is being aborted because of
statement_timeout. But then pgfdw_xact_callback() doesn't seem to have
a way to know whether this ABORT is because of user's request to
cancel the query, statement timeout, an abort because of some other
error or a user requested abort. Except statement timeout (may be
user's request to cancel the query?), it should try to keep the
connection around to avoid any future reconnection. But I am not able
to see how can we provide that information to pgfdw_xact_callback().

Expiration of statement_timeout doesn't mean a stall of foreign
connections. If we are to keep connections by, for example, a
cancel request from client, we also should keep them on
statememt_timeout because it is not necessariry triggered by a
stall of foreign connection.

When statement_timeout completes, we don't want to spend more time in
trying to cancel queries: esp when there are many foreign server, each
consuming some "timeout" time OR even trying to send Abort transaction
statement. Instead, we should slam those down. I consider this to be
different from query cancellation since query cancellation doesn't
have a hard bound on time, although we would like to cancel the
running query as fast as possible. Rethinking about it, probably we
should slam down the connection in case of query cancel as well.

Yeah, both are rather unusual, in other words, we don't expect it
to happen twice or more in a short interval. With that
prerequisite, I think slamming all down is optimal. But
defenitely not for the commanded rollback case.

I think we can detect a stall of the channel where the foreign
connections are on by a cancel request with a very short timeout,
although it is a bit incorrect.

I reconsider this problem and my proposal for this issue is as
the follows.

- Foreign servers have a new options 'stall_detection_threshold'
in milliseconds, maybe defaults to connect_timeout of the
foreign server setting. For many foreign servers in a local
network, it could be lowered to several tens of milliseconds.

A connect_timeout less than 2 seconds is not encouraged.
https://www.postgresql.org/docs/devel/static/libpq-connect.html. So,
we can not set stall_detection_threshold to be smaller than 2 seconds.

It seems coming from DNS lookup timeout so users can set more
short timeout. Anyway the several-tens or hundres millisecond is
an extreme example. But 2 seconds for every connection would be
too long. Anyway the resolution is 1 second..

statement_timeout however is set in milliseconds, so 2 seconds per
connection would be quite a lot compared to statement_timeout setting.
Waiting to cancel query for 2 seconds when the statement_timeout
itself is 2 seconds would mean the query would be cancelled after 4
seconds, which is kind of funny.

Yes, we can keep connections in the cases but in turn it can
complel a user to wait for incrediblly long time for a cancel
request. We could reduce the time to wait with some complex stuff
but I don't think we can cancel queries to hundreds of foreign
servers in a second without asynchronisity or parallelism. It
seems to me unreasonablly complex.

In short, I'd like to just discard all connections by the two
kind of triggers.

I suppose that we can inform the cause to the transaction
callback as an event, but I haven't realize how
AbortCurrentTransaction can detect the cause. Letting
ProcessInterrupts set an additional global variable such like
requested_immediate_abort could work.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
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, Apr 20, 2017 at 10:27 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

The logs above show that 34 seconds elapsed between starting to abort
the transaction and knowing that the foreign server is unreachable. It
looks like it took that much time for the local server to realise that
the foreign server is not reachable. Looking at PQcancel code, it
seems to be trying to connect to the foreign server to cancel the
query. But somehow it doesn't seem to honor connect_timeout setting.
Is that expected?

Well, there's no code to do anything else. Regular connections go
through connectDBComplete() which uses non-blocking mode and timed
waits to respect connection_timeout. PQcancel() has no such handling.
internal_cancel just opens a socket and, without setting any options
on it, calls connect(). No loop, no timed waits, nada. So it's going
to take as long as it takes for the operating system to notice.

Irrespective of what PQcancel does, it looks like postgres_fdw should
just slam the connection if query is being aborted because of
statement_timeout. But then pgfdw_xact_callback() doesn't seem to have
a way to know whether this ABORT is because of user's request to
cancel the query, statement timeout, an abort because of some other
error or a user requested abort. Except statement timeout (may be
user's request to cancel the query?), it should try to keep the
connection around to avoid any future reconnection. But I am not able
to see how can we provide that information to pgfdw_xact_callback().

I don't think we can. In general, PostgreSQL has no facility for
telling error cleanup handlers why the abort happened, and it wouldn't
really be the right thing anyway. The fact that statement_timeout
fired doesn't necessarily mean that the connection is dead; it could
equally well mean that the query ran for a long time. I think we
basically have two choices. One is to bound the amount of time we
spend performing error cleanup, and the other is just to always drop
the connection. A problem with the latter is that it might do the
wrong thing if we're aborting a subtransaction but not the whole
transaction. In that case, we need to undo changes since the relevant
savepoint, but not the ones made before that; closing the connection
amounts to a full rollback.

Therefore, I think the patch you proposed in
/messages/by-id/CAFjFpRdcWw4h0a-zrL-EiaekkPj8O0GR2M1FwZ1useSRfRm3-g@mail.gmail.com
isn't correct, because if the cancel fails it will slam the connection
shut regardless of whether we're in pgfdw_xact_callback or
pgfdw_subxact_callback.

It looks to me like there's a fairly lengthy list of conceptually
separate but practically related problems here:

1. PQcancel() ignores the keepalive parameters, because it makes no
attempt to set the relevant socket options before connecting.

2. PQcancel() ignores connection_timeout, because it doesn't use
non-blocking mode and has no wait loop.

3. There is no asynchronous equivalent of PQcancel(), so we can't use
a loop to allow responding to interrupts while the cancel is
outstanding (see pgfdw_get_result for an example).

4. pgfdw_xact_callback() and pgfdw_subxact_callback() use PQexec()
rather than the asynchronous interfaces for sending queries and
checking for results, so the SQL commands they send are not
interruptible.

5. pgfdw_xact_callback() and pgfdw_subxact_callback() try to get the
connection back to a good state by using ABORT TRANSACTION for the
former and ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT for the latter.
But if it doesn't work, then they just emit a WARNING and continue on
as if they'd succeeded. That seems highly likely to make the next use
of that connection fail in unpredictable ways.

6. postgres_fdw doesn't use PQsetnonblocking() anywhere, so even if we
converted pgfdw_xact_callback() and pgfdw_subxact_callback() to use
pgfdw_exec_query() or something like it rather than PQexec() to submit
queries, they might still block if we fail to send the query, as the
comments for pgfdw_exec_query() explain. This is not possibly but not
particularly likely to happen for queries being sent out of error
handling paths, because odds are good that the connection was sent
just before. However, if the user is pressing ^C because the remote
server isn't responding, it's quite probable that we'll run into this
exact issue.

7. postgres_fdw never considers slamming the connection shut as a
response to trouble. It seems pretty clear that this is only a safe
response if we're aborting the toplevel transaction. If we did it for
a subtransaction, we'd end up reconnecting if the server were accessed
again, which would at the very least change the snapshot (note that we
use at least REPEATABLE READ on the remote side regardless of the
local isolation level) and might discard changes made on the remote
side at outer transaction levels. Even for a top-level transaction,
it might not always be the right way to proceed, as it incurs some
overhead to reconnect, but it seems worth considering at least in the
case where we've already got some indication that the connection is
unhealthy (e.g. a previous attempt to clean up the connection state
failed, as in #5, or PQcancel failed, as in Ashutosh's proposed
patch).

8. Before 9.6, PQexec() is used in many more places, so many more
queries are entirely non-interruptible.

It seems pretty clear to me that we are not going to fix all of these
issues in one patch. Here's a sketch of an idea for how to start
making things better:

- Add an in_abort_cleanup flag to ConnCacheEntry.

- Before pgfdw_xact_callback() or pgfdw_subxact_callback() begin abort
cleanup, check whether the flag is set. If so, slam the connection
shut unless that's already been done; furthermore, if the flag is set
and we're in pgfdw_xact_callback (i.e. this is a toplevel abort),
forget about the connection entry entirely. On the other hand, if the
flag is not set, set it flag and attempt abort cleanup. If we
succeed, clear the flag.

- Before pgfdw_xact_callback() or pgfdw_subxact_callback() begin
pre-commit processing, check whether the flag is set. If so, throw an
ERROR, so that we switch over to abort processing.

- Change uses of PQexec() in the abort path to use pgfdw_exec_query()
instead. If we exit pgfdw_exec_query() with an error, we'll re-enter
the abort path, but now in_abort_cleanup will be set, so we'll just
drop the connection (and force any outer transaction levels to abort
as well).

- For bonus points, give pgfdw_exec_query() an optional timeout
argument, and set it to 30 seconds or so when we're doing abort
cleanup. If the timeout succeeds, it errors out (which again
re-enters the abort path, dropping the connection and forcing outer
transaction levels to abort as well).

Assuming the above works out, I propose to back-patch
f039eaac7131ef2a4cf63a10cf98486f8bcd09d2,
1b812afb0eafe125b820cc3b95e7ca03821aa675, and the changes above to all
supported releases. That would fix problems 4, 5, 7, and 8 from the
above list for all branches.

Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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 Wed, May 3, 2017 at 3:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:

It seems pretty clear to me that we are not going to fix all of these
issues in one patch. Here's a sketch of an idea for how to start
making things better:

Patch attached.

- Add an in_abort_cleanup flag to ConnCacheEntry.

Ended up renaming this to abort_cleanup_incomplete.

- Before pgfdw_xact_callback() or pgfdw_subxact_callback() begin abort
cleanup, check whether the flag is set. If so, slam the connection
shut unless that's already been done; furthermore, if the flag is set
and we're in pgfdw_xact_callback (i.e. this is a toplevel abort),
forget about the connection entry entirely. On the other hand, if the
flag is not set, set it flag and attempt abort cleanup. If we
succeed, clear the flag.

Did approximately this. It turned out not to be necessary to add any
new calls to PQfinish(); the existing one was adequate.

- Before pgfdw_xact_callback() or pgfdw_subxact_callback() begin
pre-commit processing, check whether the flag is set. If so, throw an
ERROR, so that we switch over to abort processing.

Did this.

- Change uses of PQexec() in the abort path to use pgfdw_exec_query()
instead. If we exit pgfdw_exec_query() with an error, we'll re-enter
the abort path, but now in_abort_cleanup will be set, so we'll just
drop the connection (and force any outer transaction levels to abort
as well).

Created a new function pgfdw_exec_cleanup_query() for this, also
incorporating a timeout.

Also fixed things so that after issuing PQcancel() we actually throw
away the pending result from the cancelled query, and added some error
recursion protection.

Review would be appreciated.

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

On 05/04/2017 08:01 AM, Robert Haas wrote:

Patch attached.

I tried at my end after applying the patch against PG HEAD,

Case 1 - without setting statement_timeout i.e default

X machine -
create table test1(a int);

Y machine -
CREATE SERVER myserver_ppas FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(host 'X', dbname 'postgres', port '5432', connect_timeout '3');
CREATE USER MAPPING FOR centos SERVER myserver_ppas OPTIONS (user
'centos', password 'adminedb');
create foreign table ft_test_ppas (a int ) server myserver_ppas options
(table_name 'test1');
statement_timeout =0;
\timing
insert into ft_test_ppas values (generate_series(1,10000000));

X machine-
disconnect network

Y machine -
postgres=# insert into ft_test_ppas values (generate_series(1,10000000));
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
^CCancel request sent
WARNING: could not send cancel request: PQcancel() -- connect() failed:
Connection timed out

ERROR: canceling statement due to user request
Time: 81073.872 ms (01:21.074)

Case 2- when statement_timeout=6000

Y machine -
CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host
'X', dbname 'postgres', port '5432',keepalives '1', keepalives_interval
'3',keepalives_idle '3', keepalives_count '1');
CREATE USER MAPPING FOR centos SERVER myserver OPTIONS (user 'centos',
password 'adminedb');
create foreign table ft_test_ppas1 (a int ) server myserver options
(table_name 'test1');
set statement_timeout=6000;
\timing
insert into ft_test_ppas1 values (generate_series(1,10000000));

X machine-
disconnect network

Y machine
postgres=# insert into ft_test_ppas1 values
(generate_series(1,10000000));
WARNING: could not send cancel request: PQcancel() -- connect() failed:
Connection timed out

ERROR: canceling statement due to statement timeout
Time: 69009.875 ms (01:09.010)
postgres=#

Case 3-when statement_timeout=20000

Y machine -
CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host
'X', dbname 'postgres', port '5432',keepalives '1', keepalives_interval
'3',keepalives_idle '3', keepalives_count '1');
CREATE USER MAPPING FOR centos SERVER myserver OPTIONS (user 'centos',
password 'adminedb');
create foreign table ft_test_ppas1 (a int ) server myserver options
(table_name 'test1');
set statement_timeout=20000;
\timing
insert into ft_test_ppas1 values (generate_series(1,10000000));

X machine-
disconnect network

Y machine -
postgres=# insert into ft_test_ppas1 values
(generate_series(1,10000000));
WARNING: could not send cancel request: PQcancel() -- connect() failed:
Connection timed out
ERROR: canceling statement due to statement timeout
Time: 83014.503 ms (01:23.015)

We can see statement_timeout is working but it is taking some extra
time,not sure this is an expected behavior in above case or not.

--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL 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 05/04/2017 03:53 PM, tushar wrote:

We can see statement_timeout is working but it is taking some extra
time,not sure this is an expected behavior in above case or not.

This is only when remote server is involved . in case when both the
servers are on the same machine , then this is working as expected.

d1=# CREATE SERVER myserver_ppas FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (host 'localhost', dbname 'postgres', port '5432',
connect_timeout '3');
CREATE SERVER
d1=# CREATE USER MAPPING FOR centos SERVER myserver_ppas OPTIONS (user
'centos', password 'adminedb');
CREATE USER MAPPING
d1=# create foreign table ft_test_ppas (a int ) server myserver_ppas
options (table_name 'test1');
CREATE FOREIGN TABLE
d1=#
d1=# insert into ft_test_ppas values (1);
INSERT 0 1
Case 1-
d1=# \timing
Timing is on.
d1=# set statement_timeout =6000;
SET
Time: 0.360 ms
d1=# insert into ft_test_ppas values (generate_series(1,10000000));
ERROR: canceling statement due to statement timeout
Time: 6002.509 ms (00:06.003)
d1=#
Case 2 -
d1=# set statement_timeout =20000;
SET
Time: 0.693 ms
d1=# insert into ft_test_ppas values (generate_series(1,10000000));
ERROR: canceling statement due to statement timeout
Time: 20001.741 ms (00:20.002)
d1=#

--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL 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, May 4, 2017 at 6:23 AM, tushar <tushar.ahuja@enterprisedb.com> wrote:

We can see statement_timeout is working but it is taking some extra time,not
sure this is an expected behavior in above case or not.

Yeah, that's expected. To fix that, we'd need libpq to have an async
equivalent of PQcancel(), which doesn't currently exist. I think it
would be a good idea to add that, but that's another discussion. With
this patch:

1. If postgres_fdw is waiting for one of the cleanup queries to
execute, you can cancel it, and things like statement_timeout will
also work.

2. If the cancel fails or any of the cleanup queries fail,
postgres_fdw will forcibly close the connection and force the current
transaction and all subtransactions to abort. This isn't wonderful
behavior, but if we can't talk to the remote server any more there
doesn't seem to be any other real alternative. (We could improve
this, I think, by tracking whether the connection had ever been use by
an outer transaction level, and if not, allowing the transaction to
commit if it never again tried to access the failed connection, but I
left the design and installation of such a mechanism to a future
patch.)

3. But if you're stuck trying to send the cancel request itself, you
still have to wait for that to fail before anything else happens.
Once it does, we'll proceed as outlined in point #2. This stinks, but
it's the inevitable consequence of having no async equivalent of
PQcancel().

My goal is to make things noticeably better with this patch, not to
fix them completely.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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, May 4, 2017 at 1:19 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Apr 20, 2017 at 10:27 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

The logs above show that 34 seconds elapsed between starting to abort
the transaction and knowing that the foreign server is unreachable. It
looks like it took that much time for the local server to realise that
the foreign server is not reachable. Looking at PQcancel code, it
seems to be trying to connect to the foreign server to cancel the
query. But somehow it doesn't seem to honor connect_timeout setting.
Is that expected?

Well, there's no code to do anything else. Regular connections go
through connectDBComplete() which uses non-blocking mode and timed
waits to respect connection_timeout. PQcancel() has no such handling.
internal_cancel just opens a socket and, without setting any options
on it, calls connect(). No loop, no timed waits, nada. So it's going
to take as long as it takes for the operating system to notice.

Irrespective of what PQcancel does, it looks like postgres_fdw should
just slam the connection if query is being aborted because of
statement_timeout. But then pgfdw_xact_callback() doesn't seem to have
a way to know whether this ABORT is because of user's request to
cancel the query, statement timeout, an abort because of some other
error or a user requested abort. Except statement timeout (may be
user's request to cancel the query?), it should try to keep the
connection around to avoid any future reconnection. But I am not able
to see how can we provide that information to pgfdw_xact_callback().

I don't think we can. In general, PostgreSQL has no facility for
telling error cleanup handlers why the abort happened, and it wouldn't
really be the right thing anyway. The fact that statement_timeout
fired doesn't necessarily mean that the connection is dead; it could
equally well mean that the query ran for a long time. I think we
basically have two choices. One is to bound the amount of time we
spend performing error cleanup, and the other is just to always drop
the connection. A problem with the latter is that it might do the
wrong thing if we're aborting a subtransaction but not the whole
transaction. In that case, we need to undo changes since the relevant
savepoint, but not the ones made before that; closing the connection
amounts to a full rollback.

Therefore, I think the patch you proposed in
/messages/by-id/CAFjFpRdcWw4h0a-zrL-EiaekkPj8O0GR2M1FwZ1useSRfRm3-g@mail.gmail.com
isn't correct, because if the cancel fails it will slam the connection
shut regardless of whether we're in pgfdw_xact_callback or
pgfdw_subxact_callback.

It looks to me like there's a fairly lengthy list of conceptually
separate but practically related problems here:

1. PQcancel() ignores the keepalive parameters, because it makes no
attempt to set the relevant socket options before connecting.

2. PQcancel() ignores connection_timeout, because it doesn't use
non-blocking mode and has no wait loop.

3. There is no asynchronous equivalent of PQcancel(), so we can't use
a loop to allow responding to interrupts while the cancel is
outstanding (see pgfdw_get_result for an example).

4. pgfdw_xact_callback() and pgfdw_subxact_callback() use PQexec()
rather than the asynchronous interfaces for sending queries and
checking for results, so the SQL commands they send are not
interruptible.

5. pgfdw_xact_callback() and pgfdw_subxact_callback() try to get the
connection back to a good state by using ABORT TRANSACTION for the
former and ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT for the latter.
But if it doesn't work, then they just emit a WARNING and continue on
as if they'd succeeded. That seems highly likely to make the next use
of that connection fail in unpredictable ways.

In pgfdw_xact_callback, if the execution of ABORT TRANSACTION fails
due to any reason then I think it will close the connection. The
relavant code is:
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE)

Basically, if abort transaction fails then transaction status won't be
PQTRANS_IDLE. Also, does it matter if pgfdw_subxact_callback fails
during execution of "ROLLBACK TO SAVEPOINT", because anyway user needs
to execute rollback to savepoint command before proceeding and that
should be good enough?

About patch:

+ /* Rollback all remote subtransactions during abort */
+ snprintf(sql, sizeof(sql),
+ "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
+ curlevel, curlevel);
+ if (!pgfdw_exec_cleanup_query(entry->conn, sql))
+ abort_cleanup_failure = true;

If the above execution fails due to any reason, then it won't allow
even committing the mail transaction which I am not sure is the right
thing to do. I have tried it manually editing the above command in
debugger and the result is as below:

Create a foreign table and try below scenario.

postgres=# begin;
BEGIN
postgres=# savepoint s1;
SAVEPOINT
postgres=# insert into foreign_test values(8);
INSERT 0 1
postgres=# savepoint s2;
SAVEPOINT
postgres=# insert into foreign_test values(generate_series(1,1000000));
Cancel request sent
WARNING: syntax error at or near "OOLLBACK"
ERROR: canceling statement due to user request

-- In the debugger, I have changed ROLLBACK to mimic the failure.

postgres=# Rollback to savepoint s2;
ROLLBACK
postgres=# commit;
ERROR: connection to server "foreign_server" was lost

Considering above, I am not sure if we should consider all failures
during abort of subtransaction to indicate pending cleanup state and
then don't allow further commits.

 #include "utils/memutils.h"
-
+#include "utils/syscache.h"

Looks like spurious line removal

6. postgres_fdw doesn't use PQsetnonblocking() anywhere, so even if we
converted pgfdw_xact_callback() and pgfdw_subxact_callback() to use
pgfdw_exec_query() or something like it rather than PQexec() to submit
queries, they might still block if we fail to send the query, as the
comments for pgfdw_exec_query() explain. This is not possibly but not
particularly likely to happen for queries being sent out of error
handling paths, because odds are good that the connection was sent
just before. However, if the user is pressing ^C because the remote
server isn't responding, it's quite probable that we'll run into this
exact issue.

7. postgres_fdw never considers slamming the connection shut as a
response to trouble. It seems pretty clear that this is only a safe
response if we're aborting the toplevel transaction. If we did it for
a subtransaction, we'd end up reconnecting if the server were accessed
again, which would at the very least change the snapshot (note that we
use at least REPEATABLE READ on the remote side regardless of the
local isolation level) and might discard changes made on the remote
side at outer transaction levels. Even for a top-level transaction,
it might not always be the right way to proceed, as it incurs some
overhead to reconnect, but it seems worth considering at least in the
case where we've already got some indication that the connection is
unhealthy (e.g. a previous attempt to clean up the connection state
failed, as in #5, or PQcancel failed, as in Ashutosh's proposed
patch).

8. Before 9.6, PQexec() is used in many more places, so many more
queries are entirely non-interruptible.

It seems pretty clear to me that we are not going to fix all of these
issues in one patch. Here's a sketch of an idea for how to start
making things better:

- Add an in_abort_cleanup flag to ConnCacheEntry.

- Before pgfdw_xact_callback() or pgfdw_subxact_callback() begin abort
cleanup, check whether the flag is set. If so, slam the connection
shut unless that's already been done; furthermore, if the flag is set
and we're in pgfdw_xact_callback (i.e. this is a toplevel abort),
forget about the connection entry entirely. On the other hand, if the
flag is not set, set it flag and attempt abort cleanup. If we
succeed, clear the flag.

- Before pgfdw_xact_callback() or pgfdw_subxact_callback() begin
pre-commit processing, check whether the flag is set. If so, throw an
ERROR, so that we switch over to abort processing.

- Change uses of PQexec() in the abort path to use pgfdw_exec_query()
instead. If we exit pgfdw_exec_query() with an error, we'll re-enter
the abort path, but now in_abort_cleanup will be set, so we'll just
drop the connection (and force any outer transaction levels to abort
as well).

- For bonus points, give pgfdw_exec_query() an optional timeout
argument, and set it to 30 seconds or so when we're doing abort
cleanup. If the timeout succeeds, it errors out (which again
re-enters the abort path, dropping the connection and forcing outer
transaction levels to abort as well).

Why do we need this 30 seconds timeout for abort cleanup failures?
Isn't it sufficient if we propagate the error only on failures?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
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, May 4, 2017 at 7:13 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

In pgfdw_xact_callback, if the execution of ABORT TRANSACTION fails
due to any reason then I think it will close the connection. The
relavant code is:
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE)

Basically, if abort transaction fails then transaction status won't be
PQTRANS_IDLE.

I don't think that's necessarily true. Look at this code:

/* Rollback all remote subtransactions during abort */
snprintf(sql, sizeof(sql),
"ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
curlevel, curlevel);
res = PQexec(entry->conn, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
pgfdw_report_error(WARNING, res, entry->conn, true, sql);
else
PQclear(res);

If that sql command somehow returns a result status other than
PGRES_COMMAND_OK, like say due to a local OOM failure or whatever, the
connection is PQTRANS_IDLE but the rollback wasn't actually done.

Also, does it matter if pgfdw_subxact_callback fails
during execution of "ROLLBACK TO SAVEPOINT", because anyway user needs
to execute rollback to savepoint command before proceeding and that
should be good enough?

I don't understand this. If pgfdw_subxact_callback doesn't roll the
remote side back to the savepoint, how is it going to get done later?
There's no code in postgres_fdw other than that code to issue the
rollback.

About patch:

+ /* Rollback all remote subtransactions during abort */
+ snprintf(sql, sizeof(sql),
+ "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
+ curlevel, curlevel);
+ if (!pgfdw_exec_cleanup_query(entry->conn, sql))
+ abort_cleanup_failure = true;

If the above execution fails due to any reason, then it won't allow
even committing the mail transaction which I am not sure is the right
thing to do.

Well, as I said in my reply to Tushar, I think it's the only correct
thing to do. Suppose you do the following:

(1) make a change to the foreign table
(2) enter a subtransaction
(3) make another change to the foreign table
(4) roll back the subtransaction
(5) commit the transaction

If the remote rollback gets skipped in step 4, it is dead wrong for
step 5 to commit the remote transaction anyway. Both the change made
in step (1) and the change made in step (3) would get made on the
remote side, which is 100% wrong. Given the failure of the rollback
in step 4, there is no way to achieve the desired outcome of
committing the step (1) change and rolling back the step (3) change.
The only way forward is to roll back everything - i.e. force a
toplevel abort.

#include "utils/memutils.h"
-
+#include "utils/syscache.h"

Looks like spurious line removal

OK.

- For bonus points, give pgfdw_exec_query() an optional timeout
argument, and set it to 30 seconds or so when we're doing abort
cleanup. If the timeout succeeds, it errors out (which again
re-enters the abort path, dropping the connection and forcing outer
transaction levels to abort as well).

Why do we need this 30 seconds timeout for abort cleanup failures?
Isn't it sufficient if we propagate the error only on failures?

Well, the problem is that right now we're waiting for vast amounts of
time when the remote connection dies. First, the user notices that
the command is running for too long and hits ^C. At that point, the
connection is probably already dead, and the user's been waiting for a
while already. Then, we wait for PQcancel() to time out. Then, we
wait for PQexec() to time out. The result of that, as Suraj mentioned
in the original email, is that it takes about 16 minutes for the
session to recover when the connection dies, even with keepalive
settings and connection timeout set all the way to the minimum. The
goal here is to make it take a more reasonable time to recover.
Without modifying libpq, we can't do anything about the need to wait
for PQcancel() to time out, but we can improve the rest of it. If we
removed that 30-second timeout, we would win in the case where either
ABORT TRANSACTION or ROLLBACK; RELEASE eventually succeeds but takes
more than 30 seconds. In that case, the change you are proposing
would allow us to reuse the connection instead of dropping it, and
would prevent a forced toplevel abort when subtransactions are in use.
However, the cost would be that when one of those commands never
succeeds, we would wait a lot longer before finishing a transaction
abort. I argue that if the remote server fails to respond to one of
those commands within 30 seconds, it's probably dead or nearly dead,
and we're probably better off just treating it as dead rather than
waiting longer. That's a judgement call, of course.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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, May 4, 2017 at 8:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, May 4, 2017 at 7:13 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

- For bonus points, give pgfdw_exec_query() an optional timeout
argument, and set it to 30 seconds or so when we're doing abort
cleanup. If the timeout succeeds, it errors out (which again
re-enters the abort path, dropping the connection and forcing outer
transaction levels to abort as well).

Why do we need this 30 seconds timeout for abort cleanup failures?
Isn't it sufficient if we propagate the error only on failures?

Well, the problem is that right now we're waiting for vast amounts of
time when the remote connection dies. First, the user notices that
the command is running for too long and hits ^C. At that point, the
connection is probably already dead, and the user's been waiting for a
while already. Then, we wait for PQcancel() to time out. Then, we
wait for PQexec() to time out. The result of that, as Suraj mentioned
in the original email, is that it takes about 16 minutes for the
session to recover when the connection dies, even with keepalive
settings and connection timeout set all the way to the minimum. The
goal here is to make it take a more reasonable time to recover.
Without modifying libpq, we can't do anything about the need to wait
for PQcancel() to time out, but we can improve the rest of it. If we
removed that 30-second timeout, we would win in the case where either
ABORT TRANSACTION or ROLLBACK; RELEASE eventually succeeds but takes
more than 30 seconds. In that case, the change you are proposing
would allow us to reuse the connection instead of dropping it, and
would prevent a forced toplevel abort when subtransactions are in use.
However, the cost would be that when one of those commands never
succeeds, we would wait a lot longer before finishing a transaction
abort.

As soon as the first command fails due to timeout, we will set
'abort_cleanup_failure' which will make a toplevel transaction to
abort and also won't allow other statements to execute. The patch is
trying to enforce a 30-second timeout after statement execution, so it
has to anyway wait till the command execution completes (irrespective
of whether the command succeeds or fail). It is quite possible I am
missing some point, is it possible for you to tell in somewhat more
detail in which exact case 30-second timeout will allow us to abort
the toplevel transaction if we already do that in the case of
statement failure case?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
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, May 4, 2017 at 12:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

As soon as the first command fails due to timeout, we will set
'abort_cleanup_failure' which will make a toplevel transaction to
abort and also won't allow other statements to execute. The patch is
trying to enforce a 30-second timeout after statement execution, so it
has to anyway wait till the command execution completes (irrespective
of whether the command succeeds or fail).

I don't understand what you mean by this. If the command doesn't
finish within 30 seconds, we *don't* wait for it to finish. To avoid
getting inconsistent results in consequence, we set
abort_cleanup_failure.

It is quite possible I am
missing some point, is it possible for you to tell in somewhat more
detail in which exact case 30-second timeout will allow us to abort
the toplevel transaction if we already do that in the case of
statement failure case?

Suppose the user's original connection is stuck for some reason but
the postmaster is still accepting new connections - e.g. somebody
attached gdb to the remote server process and it is therefore stopped.
The cancel request gets sent OK, but without the 30-second timeout,
we'll hang forever (or until gdb continues or detaches the processes)
waiting for it to take effect.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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, May 4, 2017 at 10:18 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, May 4, 2017 at 12:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

As soon as the first command fails due to timeout, we will set
'abort_cleanup_failure' which will make a toplevel transaction to
abort and also won't allow other statements to execute. The patch is
trying to enforce a 30-second timeout after statement execution, so it
has to anyway wait till the command execution completes (irrespective
of whether the command succeeds or fail).

I don't understand what you mean by this. If the command doesn't
finish within 30 seconds, we *don't* wait for it to finish.

+ /*
+ * Submit a query.  Since we don't use non-blocking mode, this also can
+ * block.  But its risk is relatively small, so we ignore that for now.
+ */
+ if (!PQsendQuery(conn, query))
+ {
+ pgfdw_report_error(WARNING, NULL, conn, false, query);
+ return false;
+ }
+
+ /* Get the result of the query. */
+ if (pgfdw_get_cleanup_result(conn, endtime, &result))
+ return false;

The 30 seconds logic is in function pgfdw_get_cleanup_result, can't
the command hang in PQsendQuery?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
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, May 4, 2017 at 1:04 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 4, 2017 at 10:18 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, May 4, 2017 at 12:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

As soon as the first command fails due to timeout, we will set
'abort_cleanup_failure' which will make a toplevel transaction to
abort and also won't allow other statements to execute. The patch is
trying to enforce a 30-second timeout after statement execution, so it
has to anyway wait till the command execution completes (irrespective
of whether the command succeeds or fail).

I don't understand what you mean by this. If the command doesn't
finish within 30 seconds, we *don't* wait for it to finish.

+ /*
+ * Submit a query.  Since we don't use non-blocking mode, this also can
+ * block.  But its risk is relatively small, so we ignore that for now.
+ */
+ if (!PQsendQuery(conn, query))
+ {
+ pgfdw_report_error(WARNING, NULL, conn, false, query);
+ return false;
+ }
+
+ /* Get the result of the query. */
+ if (pgfdw_get_cleanup_result(conn, endtime, &result))
+ return false;

The 30 seconds logic is in function pgfdw_get_cleanup_result, can't
the command hang in PQsendQuery?

Sure. I thought about trying to fix that too, by using
PQsetnonblocking(), but I thought the patch was doing enough already.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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, May 4, 2017 at 8:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, May 4, 2017 at 7:13 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

In pgfdw_xact_callback, if the execution of ABORT TRANSACTION fails
due to any reason then I think it will close the connection. The
relavant code is:
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE)

Basically, if abort transaction fails then transaction status won't be
PQTRANS_IDLE.

I don't think that's necessarily true. Look at this code:

/* Rollback all remote subtransactions during abort */
snprintf(sql, sizeof(sql),
"ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
curlevel, curlevel);
res = PQexec(entry->conn, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
pgfdw_report_error(WARNING, res, entry->conn, true, sql);
else
PQclear(res);

If that sql command somehow returns a result status other than
PGRES_COMMAND_OK, like say due to a local OOM failure or whatever, the
connection is PQTRANS_IDLE but the rollback wasn't actually done.

I have tried to verify above point and it seems to me in such a
situation the transaction status will be either PQTRANS_INTRANS or
PQTRANS_INERROR. I have tried to mimic "out of memory" failure by
making PQmakeEmptyPGresult return NULL (malloc failure). AFAIU, it
will make the state as PQTRANS_IDLE only when the statement is
executed successfully.

Also, does it matter if pgfdw_subxact_callback fails
during execution of "ROLLBACK TO SAVEPOINT", because anyway user needs
to execute rollback to savepoint command before proceeding and that
should be good enough?

I don't understand this. If pgfdw_subxact_callback doesn't roll the
remote side back to the savepoint, how is it going to get done later?
There's no code in postgres_fdw other than that code to issue the
rollback.

It is done via toplevel transaction ABORT. I think how it works is
after "ROLLBACK TO SAVEPOINT" failure, any next execution is going to
fail. Let us say if we issue Commit after failure, it will try to
execute RELEASE SAVEPOINT which will fail and lead to toplevel
transaction ABORT. Now if the toplevel transaction ABORT also fails,
it will drop the connection.

About patch:

+ /* Rollback all remote subtransactions during abort */
+ snprintf(sql, sizeof(sql),
+ "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
+ curlevel, curlevel);
+ if (!pgfdw_exec_cleanup_query(entry->conn, sql))
+ abort_cleanup_failure = true;

If the above execution fails due to any reason, then it won't allow
even committing the mail transaction which I am not sure is the right
thing to do.

Well, as I said in my reply to Tushar, I think it's the only correct
thing to do. Suppose you do the following:

(1) make a change to the foreign table
(2) enter a subtransaction
(3) make another change to the foreign table
(4) roll back the subtransaction
(5) commit the transaction

If the remote rollback gets skipped in step 4, it is dead wrong for
step 5 to commit the remote transaction anyway. Both the change made
in step (1) and the change made in step (3) would get made on the
remote side, which is 100% wrong. Given the failure of the rollback
in step 4, there is no way to achieve the desired outcome of
committing the step (1) change and rolling back the step (3) change.
The only way forward is to roll back everything - i.e. force a
toplevel abort.

Agreed, in such a situation toplevel transaction abort is the only way
out. However, it seems to me that will anyway happen in current code
even if we don't try to force it via abort_cleanup_failure. I
understand that it might be better to force it as you have done in the
patch, but not sure if it is good to drop the connection where
previously it could have done without it (for ex. if the first
statement Rollback To Savepoint fails, but ABORT succeeds). I feel it
is worth considering whether such a behavior difference is okay as you
have proposed to backpatch it.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
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, May 4, 2017 at 10:40 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, May 4, 2017 at 1:04 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 4, 2017 at 10:18 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, May 4, 2017 at 12:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

As soon as the first command fails due to timeout, we will set
'abort_cleanup_failure' which will make a toplevel transaction to
abort and also won't allow other statements to execute. The patch is
trying to enforce a 30-second timeout after statement execution, so it
has to anyway wait till the command execution completes (irrespective
of whether the command succeeds or fail).

I don't understand what you mean by this. If the command doesn't
finish within 30 seconds, we *don't* wait for it to finish.

+ /*
+ * Submit a query.  Since we don't use non-blocking mode, this also can
+ * block.  But its risk is relatively small, so we ignore that for now.
+ */
+ if (!PQsendQuery(conn, query))
+ {
+ pgfdw_report_error(WARNING, NULL, conn, false, query);
+ return false;
+ }
+
+ /* Get the result of the query. */
+ if (pgfdw_get_cleanup_result(conn, endtime, &result))
+ return false;

The 30 seconds logic is in function pgfdw_get_cleanup_result, can't
the command hang in PQsendQuery?

Sure. I thought about trying to fix that too, by using
PQsetnonblocking(), but I thought the patch was doing enough already.

Okay, it seems better to deal it in a separate patch.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

On Fri, May 5, 2017 at 1:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I have tried to verify above point and it seems to me in such a
situation the transaction status will be either PQTRANS_INTRANS or
PQTRANS_INERROR. I have tried to mimic "out of memory" failure by
making PQmakeEmptyPGresult return NULL (malloc failure). AFAIU, it
will make the state as PQTRANS_IDLE only when the statement is
executed successfully.

Hrm. OK, you might be right, but still I don't see why having the 30
second timeout is bad. People don't want a transaction rollback to
hang for a long period of time waiting for ABORT TRANSACTION to run if
we could've just dropped the connection to get the same effect. Sure,
the next transaction will then have to reconnect, but that takes a few
tens of milliseconds; if you wait for one extra second to avoid that
outcome, you come out behind even if you do manage to avoid the
reconnect. Now for a subtransaction you could argue that it's worth
being more patient, because forcing a toplevel abort sucks. But what
are the chances that, after failing to respond to a ROLLBACK; RELEASE
SAVEPOINT within 30 seconds, the remote side is going to wake up again
and start behaving normally? I'd argue that it's not terribly likely
and most people aren't going to want to wait for multiple minutes to
see whether it does, but I suppose we could impose the 30 second
timeout only at the toplevel and let subtransactions wait however long
the operating system takes to time out.

Also, does it matter if pgfdw_subxact_callback fails
during execution of "ROLLBACK TO SAVEPOINT", because anyway user needs
to execute rollback to savepoint command before proceeding and that
should be good enough?

I don't understand this. If pgfdw_subxact_callback doesn't roll the
remote side back to the savepoint, how is it going to get done later?
There's no code in postgres_fdw other than that code to issue the
rollback.

It is done via toplevel transaction ABORT. I think how it works is
after "ROLLBACK TO SAVEPOINT" failure, any next execution is going to
fail. Let us say if we issue Commit after failure, it will try to
execute RELEASE SAVEPOINT which will fail and lead to toplevel
transaction ABORT. Now if the toplevel transaction ABORT also fails,
it will drop the connection.

Mmmph. I guess that would work, but I think it's better to track it
explicitly. I tried this (bar is a foreign table):

begin;
select * from bar;
savepoint s1;
select * from bar;
savepoint s2;
select * from bar;
savepoint s3;
select * from bar;
commit;

On the remote side, the commit statement produces this:

2017-05-05 09:28:52.597 EDT [80263] LOG: statement: RELEASE SAVEPOINT s4
2017-05-05 09:28:52.597 EDT [80263] LOG: statement: RELEASE SAVEPOINT s3
2017-05-05 09:28:52.597 EDT [80263] LOG: statement: RELEASE SAVEPOINT s2
2017-05-05 09:28:52.597 EDT [80263] LOG: statement: COMMIT TRANSACTION

But that's obviously silly. Somebody could easily come along and
optimize that by getting rid of the RELEASE SAVEPOINT commands which
are completely unnecessary if we're going to commit the toplevel
transaction anyway, and then the argument that you're making wouldn't
be true any more. It seems smart to me to explicitly track whether
the remote side is known to be in a bad state rather than relying on
the future sequence of commands to be such that we'll get the right
result anyway.

Agreed, in such a situation toplevel transaction abort is the only way
out. However, it seems to me that will anyway happen in current code
even if we don't try to force it via abort_cleanup_failure. I
understand that it might be better to force it as you have done in the
patch, but not sure if it is good to drop the connection where
previously it could have done without it (for ex. if the first
statement Rollback To Savepoint fails, but ABORT succeeds). I feel it
is worth considering whether such a behavior difference is okay as you
have proposed to backpatch it.

Well, I think the whole point of this patch is that if one command on
a connection fails, the chances of future commands succeeding are
pretty poor. It's not impossible, but it's not very likely either.
To recap, the problem at present is that if network connectivity is
interrupted and the statement is then killed by statement_timeout or a
local cancel request, we try to send a cancel request to the remote
side, which takes a while to time out. Even if that fails, we then
send an ABORT TRANSACTION, hoping against hope that it will succeed
despite the failure of the cancel request. The user ends up waiting
for the cancel request to time out and then for the ABORT TRANSACTION
to time out, which takes 15-16 minutes no matter how you set the
keepalive settings and no matter how many times you hit ^C on the
local side.

Now, if you want to allow for the possibility that some future
operation might succeed never mind past failures, then you're
basically saying that trying the ABORT TRANSACTION even though the
query cancel has failed is the correct behavior. And if you don't
want the ABORT TRANSACTION to have a 30 second timeout either, then
you're saying we should wait however long it takes the operating
system to detect the failure, which again is the current behavior. If
we rip those things out of the patch, then there's not much left of
the patch. The only thing that would left would be making the wait
for ABORT TRANSACTION interruptible, so that if you hit ^C *again*
after you've already canceled the query, we then give up on waiting
for the ABORT TRANSACTION. I guess that would be an improvement over
the status quo, but it's not really a solution. People don't expect
to have to send multiple cancel requests to kill the same query in a
timely fashion, and if the commands are being sent by an application
rather than by a human being it probably won't happen.

Do you want to propose a competing patch to fix this problem? I'm
happy to listen to suggestions. But I think if you're determined to
say "let's rely entirely on the OS timeouts rather than having any of
our own" and you're also determined to say "let's not assume that a
failure of one operation means that the connection is dead", then
you're basically arguing that there's nothing really broken here, and
I don't agree with that. Aborting a transaction shouldn't take a
quarter of an hour.

You could perhaps argue that it's better to make only one of the two
changes I'm proposing and not both, but I don't really see it. If you
make the abort_cleanup_failure changes but don't apply the 30 second
timeout, then suppose the cancel request works but the connection is
dead and the ABORT TRANSACTION command takes five minutes to time out.
Well, if you'd had the timeout, you would have just closed the
connection after 30 seconds and the only thing it would have cost you
is a reconnect the next time that foreign server was used. Instead,
you waited 5 minutes in the hope of saving a reconnect cycle that
probably would have taken tens of milliseconds or less. Blech. On
the other hand, if you make the 30 second timeout changes but not the
abort_cleanup_failure changes, then suppose network connectivity is
entirely interrupted. Then you'll wait however long it takes for the
cancel to succeed plus another 30 seconds for the ABORT TRANSACTION,
whereas with the patch as proposed you only wait for the first one.
Also blech.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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 Fri, May 5, 2017 at 7:44 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, May 5, 2017 at 1:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I have tried to verify above point and it seems to me in such a
situation the transaction status will be either PQTRANS_INTRANS or
PQTRANS_INERROR. I have tried to mimic "out of memory" failure by
making PQmakeEmptyPGresult return NULL (malloc failure). AFAIU, it
will make the state as PQTRANS_IDLE only when the statement is
executed successfully.

Hrm. OK, you might be right, but still I don't see why having the 30
second timeout is bad. People don't want a transaction rollback to
hang for a long period of time waiting for ABORT TRANSACTION to run if
we could've just dropped the connection to get the same effect. Sure,
the next transaction will then have to reconnect, but that takes a few
tens of milliseconds; if you wait for one extra second to avoid that
outcome, you come out behind even if you do manage to avoid the
reconnect. Now for a subtransaction you could argue that it's worth
being more patient, because forcing a toplevel abort sucks. But what
are the chances that, after failing to respond to a ROLLBACK; RELEASE
SAVEPOINT within 30 seconds, the remote side is going to wake up again
and start behaving normally? I'd argue that it's not terribly likely
and most people aren't going to want to wait for multiple minutes to
see whether it does, but I suppose we could impose the 30 second
timeout only at the toplevel and let subtransactions wait however long
the operating system takes to time out.

Also, does it matter if pgfdw_subxact_callback fails
during execution of "ROLLBACK TO SAVEPOINT", because anyway user needs
to execute rollback to savepoint command before proceeding and that
should be good enough?

I don't understand this. If pgfdw_subxact_callback doesn't roll the
remote side back to the savepoint, how is it going to get done later?
There's no code in postgres_fdw other than that code to issue the
rollback.

It is done via toplevel transaction ABORT. I think how it works is
after "ROLLBACK TO SAVEPOINT" failure, any next execution is going to
fail. Let us say if we issue Commit after failure, it will try to
execute RELEASE SAVEPOINT which will fail and lead to toplevel
transaction ABORT. Now if the toplevel transaction ABORT also fails,
it will drop the connection.

Mmmph. I guess that would work, but I think it's better to track it
explicitly. I tried this (bar is a foreign table):

begin;
select * from bar;
savepoint s1;
select * from bar;
savepoint s2;
select * from bar;
savepoint s3;
select * from bar;
commit;

On the remote side, the commit statement produces this:

2017-05-05 09:28:52.597 EDT [80263] LOG: statement: RELEASE SAVEPOINT s4
2017-05-05 09:28:52.597 EDT [80263] LOG: statement: RELEASE SAVEPOINT s3
2017-05-05 09:28:52.597 EDT [80263] LOG: statement: RELEASE SAVEPOINT s2
2017-05-05 09:28:52.597 EDT [80263] LOG: statement: COMMIT TRANSACTION

But that's obviously silly. Somebody could easily come along and
optimize that by getting rid of the RELEASE SAVEPOINT commands which
are completely unnecessary if we're going to commit the toplevel
transaction anyway, and then the argument that you're making wouldn't
be true any more. It seems smart to me to explicitly track whether
the remote side is known to be in a bad state rather than relying on
the future sequence of commands to be such that we'll get the right
result anyway.

Sure, tracking explicitly might be a better way to do deal with it,
but at the cost of always dropping the connection in case of error
might not be the best thing to do.

Agreed, in such a situation toplevel transaction abort is the only way
out. However, it seems to me that will anyway happen in current code
even if we don't try to force it via abort_cleanup_failure. I
understand that it might be better to force it as you have done in the
patch, but not sure if it is good to drop the connection where
previously it could have done without it (for ex. if the first
statement Rollback To Savepoint fails, but ABORT succeeds). I feel it
is worth considering whether such a behavior difference is okay as you
have proposed to backpatch it.

Well, I think the whole point of this patch is that if one command on
a connection fails, the chances of future commands succeeding are
pretty poor. It's not impossible, but it's not very likely either.
To recap, the problem at present is that if network connectivity is
interrupted and the statement is then killed by statement_timeout or a
local cancel request, we try to send a cancel request to the remote
side, which takes a while to time out. Even if that fails, we then
send an ABORT TRANSACTION, hoping against hope that it will succeed
despite the failure of the cancel request. The user ends up waiting
for the cancel request to time out and then for the ABORT TRANSACTION
to time out, which takes 15-16 minutes no matter how you set the
keepalive settings and no matter how many times you hit ^C on the
local side.

Now, if you want to allow for the possibility that some future
operation might succeed never mind past failures, then you're
basically saying that trying the ABORT TRANSACTION even though the
query cancel has failed is the correct behavior. And if you don't
want the ABORT TRANSACTION to have a 30 second timeout either, then
you're saying we should wait however long it takes the operating
system to detect the failure, which again is the current behavior. If
we rip those things out of the patch, then there's not much left of
the patch.

I am not saying to rip those changes. Your most of the mail stresses
about the 30-second timeout which you have added in the patch to
detect timeouts during failures (rollback processing). I have no
objection to that part of the patch except that still there is a race
condition around PQsendQuery and you indicate that it is better to
deal it as a separate patch to which I agree. The point of which I am
not in total agreement is about the failure other than the time out.

+pgfdw_exec_cleanup_query(PGconn *conn, const char *query)
{
..
+ /* Get the result of the query. */
+ if (pgfdw_get_cleanup_result(conn, endtime, &result))
+ return false;
+
+ /* Issue a warning if not successful. */
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
+ {
+ pgfdw_report_error(WARNING, result, conn, true, query);
+ return false;
+ }
..
}

In the above code, if there is a failure due to timeout then it will
return from first statement (pgfdw_get_cleanup_result), however, if
there is any other failure, then it will issue a warning and return
false. I am not sure if there is a need to return false in the second
case, because even without that it will work fine (and there is a
chance that it won't drop the connection), but maybe your point is
better to be consistent for all kind of error handling in this path.

The only thing that would left would be making the wait
for ABORT TRANSACTION interruptible, so that if you hit ^C *again*
after you've already canceled the query, we then give up on waiting
for the ABORT TRANSACTION. I guess that would be an improvement over
the status quo, but it's not really a solution. People don't expect
to have to send multiple cancel requests to kill the same query in a
timely fashion, and if the commands are being sent by an application
rather than by a human being it probably won't happen.

Do you want to propose a competing patch to fix this problem? I'm
happy to listen to suggestions. But I think if you're determined to
say "let's rely entirely on the OS timeouts rather than having any of
our own" and you're also determined to say "let's not assume that a
failure of one operation means that the connection is dead", then
you're basically arguing that there's nothing really broken here, and
I don't agree with that.

No, I am not saying any of those. I hope the previous point clarifies
what I want to say.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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