public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jelte Fennema-Nio <[email protected]>
To: Noah Misch <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Denis Laxalde <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Greg Stark <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Gregory Stark (as CFM) <[email protected]>
Cc: Jelte Fennema <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
Date: Wed, 10 Jul 2024 14:10:55 +0200
Message-ID: <CAGECzQQO8Cn2Rw45xUYmvzXeSSsst7-bcruuzUfMbGQc3ueSdw@mail.gmail.com> (raw)
In-Reply-To: <CAGECzQRpoXw+L5Rq6j5CCGgW80+o6HP1ChJwrASQ8QonftVyFg@mail.gmail.com>
References: <CAGECzQRbCq7y7TDBFao21ngnUw5VsBgdFKQ0QJXFCeNwSyf-+Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAGECzQRpoXw+L5Rq6j5CCGgW80+o6HP1ChJwrASQ8QonftVyFg@mail.gmail.com>
On Mon, 1 Jul 2024 at 00:38, Jelte Fennema-Nio <[email protected]> wrote:
> Ugh yes, I think this was a copy paste error. See attached patch 0003
> to fix this (rest of the patches are untouched from previous
> revision).
Alvaro committed 0003, which caused cfbot to think a rebase is
necessary. Attached should solve that.
Attachments:
[application/octet-stream] v4-0001-Make-postgres_fdw-cancel-test-not-flaky-anymore.patch (6.2K, ../CAGECzQQO8Cn2Rw45xUYmvzXeSSsst7-bcruuzUfMbGQc3ueSdw@mail.gmail.com/2-v4-0001-Make-postgres_fdw-cancel-test-not-flaky-anymore.patch)
download | inline diff:
From fb6db7ba6e756b37e17096f21fc3b39085fd2585 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Tue, 25 Jun 2024 10:11:02 +0200
Subject: [PATCH v4 1/2] Make postgres_fdw cancel test not flaky anymore
The postgres_fdw cancel test turned out to be flaky. The reason for this
was that the cancel was sometimes sent earlier than intended. It was
meant to be sent during the actual CROSS JOIN, but (especially on slow
systems) it was sometimes sent in between two queries.
This patch tries to remove that issue in two ways:
1. Reduce the amount of queries that are sent by postgres_fdw, by
placing the test before enabling use_remote_estimate on any of the
tables in question.
2. Increasing the statement_timeout to 100ms
Reported-By: Alexander Lakhin
---
.../postgres_fdw/expected/postgres_fdw.out | 35 ++++++++++---------
contrib/postgres_fdw/sql/postgres_fdw.sql | 19 +++++-----
2 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1f22309194..7fa1dd3907 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -250,6 +250,25 @@ SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should work again
(1 row)
\set VERBOSITY default
+-- Let's test canceling a remote query, we do this before enabling
+-- use_remote_estimate on ft2 to avoid sending many queries to the remote
+-- server. Otherwise, we might be unlucky and the cancel to the remote might be
+-- sent right inbetween two of the queries, causing a flaky test. First let's
+-- confirm that the query is actually pushed down.
+EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: (count(*))
+ Relations: Aggregate on ((((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5))
+ Remote SQL: SELECT count(*) FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (TRUE)) INNER JOIN "S 1"."T 3" r4 ON (TRUE)) INNER JOIN "S 1"."T 4" r6 ON (TRUE))
+(4 rows)
+
+BEGIN;
+SET LOCAL statement_timeout = '100ms';
+select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
+ERROR: canceling statement due to statement timeout
+COMMIT;
-- Now we should be able to run ANALYZE.
-- To exercise multiple code paths, we use local stats on ft1
-- and remote-estimate mode on ft2.
@@ -2760,22 +2779,6 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
(10 rows)
ALTER VIEW v4 OWNER TO regress_view_owner;
--- Make sure this big CROSS JOIN query is pushed down
-EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
- QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------------------------------------------------
- Foreign Scan
- Output: (count(*))
- Relations: Aggregate on ((((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5))
- Remote SQL: SELECT count(*) FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (TRUE)) INNER JOIN "S 1"."T 3" r4 ON (TRUE)) INNER JOIN "S 1"."T 4" r6 ON (TRUE))
-(4 rows)
-
--- Make sure query cancellation works
-BEGIN;
-SET LOCAL statement_timeout = '10ms';
-select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
-ERROR: canceling statement due to statement timeout
-COMMIT;
-- ====================================================================
-- Check that userid to use when querying the remote table is correctly
-- propagated into foreign rels present in subqueries under an UNION ALL
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index b57f8cfda6..f9af38b44b 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -242,6 +242,17 @@ ALTER USER MAPPING FOR CURRENT_USER SERVER loopback
SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should work again
\set VERBOSITY default
+-- Let's test canceling a remote query, we do this before enabling
+-- use_remote_estimate on ft2 to avoid sending many queries to the remote
+-- server. Otherwise, we might be unlucky and the cancel to the remote might be
+-- sent right inbetween two of the queries, causing a flaky test. First let's
+-- confirm that the query is actually pushed down.
+EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
+BEGIN;
+SET LOCAL statement_timeout = '100ms';
+select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
+COMMIT;
+
-- Now we should be able to run ANALYZE.
-- To exercise multiple code paths, we use local stats on ft1
-- and remote-estimate mode on ft2.
@@ -742,14 +753,6 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10;
ALTER VIEW v4 OWNER TO regress_view_owner;
--- Make sure this big CROSS JOIN query is pushed down
-EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5;
--- Make sure query cancellation works
-BEGIN;
-SET LOCAL statement_timeout = '10ms';
-select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long
-COMMIT;
-
-- ====================================================================
-- Check that userid to use when querying the remote table is correctly
-- propagated into foreign rels present in subqueries under an UNION ALL
base-commit: 05506510de6ae24ba6de00cef2f458920c8a72ea
--
2.34.1
[application/octet-stream] v4-0002-Do-not-reset-statement_timeout-indicator-outside-.patch (2.2K, ../CAGECzQQO8Cn2Rw45xUYmvzXeSSsst7-bcruuzUfMbGQc3ueSdw@mail.gmail.com/3-v4-0002-Do-not-reset-statement_timeout-indicator-outside-.patch)
download | inline diff:
From b87758a8bbfde9a035de65bf9048c1521afd4ca7 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Mon, 24 Jun 2024 00:29:39 +0200
Subject: [PATCH v4 2/2] Do not reset statement_timeout indicator outside of
ProcessInterupts
The only way that ProcessInterupts can know why QueryCancelPending is
set is by looking at the indicator bits of the various timeouts. We were
resetting the one for STATEMENT_TIMEOUT in various places, thus possibly
causing ProcessInterupts to fail with the wrong error message.
---
src/backend/tcop/postgres.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e39c6804a7..03d1eb63ff 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -5150,22 +5150,33 @@ enable_statement_timeout(void)
if (StatementTimeout > 0
&& (StatementTimeout < TransactionTimeout || TransactionTimeout == 0))
{
- if (!get_timeout_active(STATEMENT_TIMEOUT))
+ /*
+ * We check both if it's active or if it's already triggered. If it's
+ * already triggered we don't want to restart it because that clears
+ * the indicator flag, which in turn would cause the wrong error
+ * message to be used by ProcessInterrupts() on the next
+ * CHECK_FOR_INTERRUPTS() call. Restarting the timer in that case
+ * would be pointless anyway, because the statement timeout error is
+ * going to trigger on the next CHECK_FOR_INTERRUPTS() call.
+ */
+ if (!get_timeout_active(STATEMENT_TIMEOUT)
+ && !get_timeout_indicator(STATEMENT_TIMEOUT, false))
enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
}
else
{
- if (get_timeout_active(STATEMENT_TIMEOUT))
- disable_timeout(STATEMENT_TIMEOUT, false);
+ disable_statement_timeout();
}
}
/*
- * Disable statement timeout, if active.
+ * Disable statement timeout, if active. We preserve the indicator flag
+ * though, otherwise we'd lose the knowledge in ProcessInterupts that the
+ * SIGINT came from a statement timeout.
*/
static void
disable_statement_timeout(void)
{
if (get_timeout_active(STATEMENT_TIMEOUT))
- disable_timeout(STATEMENT_TIMEOUT, false);
+ disable_timeout(STATEMENT_TIMEOUT, true);
}
--
2.34.1
view thread (3+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
In-Reply-To: <CAGECzQQO8Cn2Rw45xUYmvzXeSSsst7-bcruuzUfMbGQc3ueSdw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox