Invalid remote sampling test in postgres_fdw.
Patch 0001 concerns what appears to be a bug in the postgres_fdw regression
tests.
As it was, the tests set the sampling method for the foreign server, but
rather than analyzing a table on that foreign server, the local table
analyze_table was analyzed instead.
The analyze commands now analyze a remote table, and the remote table
definition links it to the local table that was previously being analyzed
repeatedly.
Patch 0002 isn't a bug, just an annoyance.
It creates two psql variables which can then be interpolated into SQL
commands as string literals, eliminating the need to wrap commands in a DO
block. This makes the queries a bit more readable, and allows for any
errors (however unlikely) in those commands to be reported individually,
rather than grouped and potentially obfuscated by the DO block.
Attachments:
v1-0001-Fix-remote-sampling-tests-in-postgres_fdw.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-remote-sampling-tests-in-postgres_fdw.patchDownload
From 7235ca80d3831d62700f7ba154112b207a266a7f Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huinker@gmail.com>
Date: Mon, 11 Aug 2025 10:05:08 -0400
Subject: [PATCH v1 1/2] Fix remote sampling tests in postgres_fdw.
These tests were changing the sampling setting of a foreign server, but
then were analyzing a local table, which doesn't actually test the
sampling.
Changed the ANALYZE commands to analyze the foreign table, and changed
the foreign table definition to point to a valid local table.
---
contrib/postgres_fdw/expected/postgres_fdw.out | 12 ++++++------
contrib/postgres_fdw/sql/postgres_fdw.sql | 12 ++++++------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index a434eb1395e..d3323b04676 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -12649,7 +12649,7 @@ ALTER SERVER loopback2 OPTIONS (DROP parallel_abort);
-- ===================================================================
CREATE TABLE analyze_table (id int, a text, b bigint);
CREATE FOREIGN TABLE analyze_ftable (id int, a text, b bigint)
- SERVER loopback OPTIONS (table_name 'analyze_rtable1');
+ SERVER loopback OPTIONS (table_name 'analyze_table');
INSERT INTO analyze_table (SELECT x FROM generate_series(1,1000) x);
ANALYZE analyze_table;
SET default_statistics_target = 10;
@@ -12657,15 +12657,15 @@ ANALYZE analyze_table;
ALTER SERVER loopback OPTIONS (analyze_sampling 'invalid');
ERROR: invalid value for string option "analyze_sampling": invalid
ALTER SERVER loopback OPTIONS (analyze_sampling 'auto');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
ALTER SERVER loopback OPTIONS (SET analyze_sampling 'system');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
ALTER SERVER loopback OPTIONS (SET analyze_sampling 'bernoulli');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
ALTER SERVER loopback OPTIONS (SET analyze_sampling 'random');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
ALTER SERVER loopback OPTIONS (SET analyze_sampling 'off');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
-- cleanup
DROP FOREIGN TABLE analyze_ftable;
DROP TABLE analyze_table;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index d9bed565c81..2c609e060b7 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -4365,7 +4365,7 @@ ALTER SERVER loopback2 OPTIONS (DROP parallel_abort);
CREATE TABLE analyze_table (id int, a text, b bigint);
CREATE FOREIGN TABLE analyze_ftable (id int, a text, b bigint)
- SERVER loopback OPTIONS (table_name 'analyze_rtable1');
+ SERVER loopback OPTIONS (table_name 'analyze_table');
INSERT INTO analyze_table (SELECT x FROM generate_series(1,1000) x);
ANALYZE analyze_table;
@@ -4376,19 +4376,19 @@ ANALYZE analyze_table;
ALTER SERVER loopback OPTIONS (analyze_sampling 'invalid');
ALTER SERVER loopback OPTIONS (analyze_sampling 'auto');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
ALTER SERVER loopback OPTIONS (SET analyze_sampling 'system');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
ALTER SERVER loopback OPTIONS (SET analyze_sampling 'bernoulli');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
ALTER SERVER loopback OPTIONS (SET analyze_sampling 'random');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
ALTER SERVER loopback OPTIONS (SET analyze_sampling 'off');
-ANALYZE analyze_table;
+ANALYZE analyze_ftable;
-- cleanup
DROP FOREIGN TABLE analyze_ftable;
base-commit: 70693c645f6e490b9ed450e8611e94ab7af3aad2
--
2.50.1
v1-0002-Use-psql-vars-to-eliminate-the-need-for-DO-blocks.patchtext/x-patch; charset=US-ASCII; name=v1-0002-Use-psql-vars-to-eliminate-the-need-for-DO-blocks.patchDownload
From 143c07bffcfeae4c2bb34d7a54370e04094284a2 Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huinker@gmail.com>
Date: Sun, 10 Aug 2025 19:36:52 -0400
Subject: [PATCH v1 2/2] Use psql vars to eliminate the need for DO blocks.
Several statements need to reference the current connection's current
database name and current port value. Until now, this has been
accomplished by creating dynamic SQL statements inside of a DO block,
which isn't as easy to read, and takes away some of the granularity of
any error messages that might occur, however unlikely.
By capturing the connection-specific settings into psql vars, it becomes
possible to write the SQL statements with simple :'varname'
interpolations.
---
.../postgres_fdw/expected/postgres_fdw.out | 44 ++++++------------
contrib/postgres_fdw/sql/postgres_fdw.sql | 46 ++++++-------------
2 files changed, 29 insertions(+), 61 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d3323b04676..6af35d04a4e 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2,23 +2,16 @@
-- create FDW objects
-- ===================================================================
CREATE EXTENSION postgres_fdw;
+SELECT current_database() AS current_database,
+ current_setting('port') AS current_port
+\gset
CREATE SERVER testserver1 FOREIGN DATA WRAPPER postgres_fdw;
-DO $d$
- BEGIN
- EXECUTE $$CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
- EXECUTE $$CREATE SERVER loopback2 FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
- EXECUTE $$CREATE SERVER loopback3 FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
- END;
-$d$;
+CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (dbname :'current_database', port :'current_port');
+CREATE SERVER loopback2 FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (dbname :'current_database', port :'current_port');
+CREATE SERVER loopback3 FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (dbname :'current_database', port :'current_port');
CREATE USER MAPPING FOR public SERVER testserver1
OPTIONS (user 'value', password 'value');
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
@@ -235,12 +228,7 @@ SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should work
ALTER SERVER loopback OPTIONS (SET dbname 'no such database');
SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should fail
ERROR: could not connect to server "loopback"
-DO $d$
- BEGIN
- EXECUTE $$ALTER SERVER loopback
- OPTIONS (SET dbname '$$||current_database()||$$')$$;
- END;
-$d$;
+ALTER SERVER loopback OPTIONS (SET dbname :'current_database');
SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should work again
c3 | c4
-------+------------------------------
@@ -10643,14 +10631,8 @@ SHOW is_superuser;
(1 row)
-- This will be OK, we can create the FDW
-DO $d$
- BEGIN
- EXECUTE $$CREATE SERVER loopback_nopw FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
- END;
-$d$;
+CREATE SERVER loopback_nopw FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (dbname :'current_database', port :'current_port');
-- But creation of user mappings for non-superusers should fail
CREATE USER MAPPING FOR public SERVER loopback_nopw;
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback_nopw;
@@ -12724,3 +12706,5 @@ SELECT server_name,
-- Clean up
\set VERBOSITY default
RESET debug_discard_caches;
+\unset current_database
+\unset current_port
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 2c609e060b7..63bba3982cb 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -4,24 +4,17 @@
CREATE EXTENSION postgres_fdw;
-CREATE SERVER testserver1 FOREIGN DATA WRAPPER postgres_fdw;
-DO $d$
- BEGIN
- EXECUTE $$CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
- EXECUTE $$CREATE SERVER loopback2 FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
- EXECUTE $$CREATE SERVER loopback3 FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
- END;
-$d$;
+SELECT current_database() AS current_database,
+ current_setting('port') AS current_port
+\gset
+CREATE SERVER testserver1 FOREIGN DATA WRAPPER postgres_fdw;
+CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (dbname :'current_database', port :'current_port');
+CREATE SERVER loopback2 FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (dbname :'current_database', port :'current_port');
+CREATE SERVER loopback3 FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (dbname :'current_database', port :'current_port');
CREATE USER MAPPING FOR public SERVER testserver1
OPTIONS (user 'value', password 'value');
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
@@ -233,12 +226,7 @@ ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should work
ALTER SERVER loopback OPTIONS (SET dbname 'no such database');
SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should fail
-DO $d$
- BEGIN
- EXECUTE $$ALTER SERVER loopback
- OPTIONS (SET dbname '$$||current_database()||$$')$$;
- END;
-$d$;
+ALTER SERVER loopback OPTIONS (SET dbname :'current_database');
SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should work again
-- Test that alteration of user mapping options causes reconnection
@@ -3375,14 +3363,8 @@ SET ROLE regress_nosuper;
SHOW is_superuser;
-- This will be OK, we can create the FDW
-DO $d$
- BEGIN
- EXECUTE $$CREATE SERVER loopback_nopw FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
- END;
-$d$;
+CREATE SERVER loopback_nopw FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (dbname :'current_database', port :'current_port');
-- But creation of user mappings for non-superusers should fail
CREATE USER MAPPING FOR public SERVER loopback_nopw;
@@ -4435,3 +4417,5 @@ SELECT server_name,
-- Clean up
\set VERBOSITY default
RESET debug_discard_caches;
+\unset current_database
+\unset current_port
--
2.50.1
On Mon, Aug 11, 2025 at 10:23:47AM -0400, Corey Huinker wrote:
Patch 0001 concerns what appears to be a bug in the postgres_fdw regression
tests.As it was, the tests set the sampling method for the foreign server, but
rather than analyzing a table on that foreign server, the local table
analyze_table was analyzed instead.The analyze commands now analyze a remote table, and the remote table
definition links it to the local table that was previously being analyzed
repeatedly.
CREATE FOREIGN TABLE analyze_ftable (id int, a text, b bigint)
- SERVER loopback OPTIONS (table_name 'analyze_rtable1');
+ SERVER loopback OPTIONS (table_name 'analyze_table');Good catch. So what your patch is telling here is that
analyze_rtable1 does not exist. I suspect a fuzz from a rebase. Will
backpatch accordingly once the stable branches reopen.
Patch 0002 isn't a bug, just an annoyance.
It creates two psql variables which can then be interpolated into SQL
commands as string literals, eliminating the need to wrap commands in a DO
block. This makes the queries a bit more readable, and allows for any
errors (however unlikely) in those commands to be reported individually,
rather than grouped and potentially obfuscated by the DO block.
Agreed that using variables leads to a more readable result. However,
I can see an argument against your proposal. It's worth noting that
Sawada-san has been relying on the trick with the DO block a couple of
days ago to get an isolation test working, inspired from the existing
SQL test:
/messages/by-id/CAD21AoCm1XYSD_R1capjuvLZ_3fKXnCu0C751Vk3hVOBDSwaCQ@mail.gmail.com
I would suggest waiting a bit for the resolution of the other bug, and
make sure that we have at least one reference to the trick with the DO
block. While the way you are writing the test is more readable,
there's also a case for keeping documented this trick in the tree in
the shape of a SQL sequence that does not depend on \gset and psql.
--
Michael
On Tue, Aug 12, 2025 at 08:58:47AM +0900, Michael Paquier wrote:
CREATE FOREIGN TABLE analyze_ftable (id int, a text, b bigint) - SERVER loopback OPTIONS (table_name 'analyze_rtable1'); + SERVER loopback OPTIONS (table_name 'analyze_table');Good catch. So what your patch is telling here is that
analyze_rtable1 does not exist. I suspect a fuzz from a rebase. Will
backpatch accordingly once the stable branches reopen.
Applied 0001 down to v16 to fix the test, as of 327bd6111aed.
--
Michael
On Tue, Aug 12, 2025 at 08:58:47AM +0900, Michael Paquier wrote:
I would suggest waiting a bit for the resolution of the other bug, and
make sure that we have at least one reference to the trick with the DO
block. While the way you are writing the test is more readable,
there's also a case for keeping documented this trick in the tree in
the shape of a SQL sequence that does not depend on \gset and psql.
I was looking again an this thread, and this argument feels a bit off
because we have a test already posted on the thread, and there would
be some past history. There is also the ease of debugging failures
when the commands fail in the DO block. At the end, applied 0002 on
HEAD.
--
Michael