Skip to content

Commit e5a3c9d

Browse files
author
Etsuro Fujita
committed
postgres_fdw: Inherit the local transaction's access/deferrable modes.
Previously, postgres_fdw always 1) opened a remote transaction in READ WRITE mode even when the local transaction was READ ONLY, causing a READ ONLY transaction using it that references a foreign table mapped to a remote view executing a volatile function to write in the remote side, and 2) opened the remote transaction in NOT DEFERRABLE mode even when the local transaction was DEFERRABLE, causing a SERIALIZABLE READ ONLY DEFERRABLE transaction using it to abort due to a serialization failure in the remote side. To avoid these, modify postgres_fdw to open a remote transaction in the same access/deferrable modes as the local transaction. This commit also modifies it to open a remote subtransaction in the same access mode as the local subtransaction. Although these issues exist since the introduction of postgres_fdw, there have been no reports from the field. So it seems fine to just fix them in master only. Author: Etsuro Fujita <etsuro.fujita@gmail.com> Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CAPmGK16n_hcUUWuOdmeUS%2Bw4Q6dZvTEDHb%3DOP%3D5JBzo-M3QmpQ%40mail.gmail.com
1 parent b006bcd commit e5a3c9d

File tree

6 files changed

+347
-8
lines changed

6 files changed

+347
-8
lines changed

‎contrib/postgres_fdw/connection.c

Lines changed: 91 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ typedef struct ConnCacheEntry
5858
/* Remaining fields are invalid when conn is NULL: */
5959
int xact_depth; /* 0 = no xact open, 1 = main xact open, 2 =
6060
* one level of subxact open, etc */
61+
bool xact_read_only; /* xact r/o state */
6162
bool have_prep_stmt; /* have we prepared any stmts in this xact? */
6263
bool have_error; /* have any subxacts aborted in this xact? */
6364
bool changing_xact_state; /* xact state change in process */
@@ -84,6 +85,12 @@ static unsigned int prep_stmt_number = 0;
8485
/* tracks whether any work is needed in callback functions */
8586
static bool xact_got_connection = false;
8687

88+
/*
89+
* tracks the nesting level of the topmost read-only transaction determined
90+
* by GetTopReadOnlyTransactionNestLevel()
91+
*/
92+
static int top_read_only_level = 0;
93+
8794
/* custom wait event values, retrieved from shared memory */
8895
static uint32 pgfdw_we_cleanup_result = 0;
8996
static uint32 pgfdw_we_connect = 0;
@@ -372,6 +379,7 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user)
372379

373380
/* Reset all transient state fields, to be sure all are clean */
374381
entry->xact_depth = 0;
382+
entry->xact_read_only = false;
375383
entry->have_prep_stmt = false;
376384
entry->have_error = false;
377385
entry->changing_xact_state = false;
@@ -843,29 +851,81 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input)
843851
* those scans. A disadvantage is that we can't provide sane emulation of
844852
* READ COMMITTED behavior --- it would be nice if we had some other way to
845853
* control which remote queries share a snapshot.
854+
*
855+
* Note also that we always start the remote transaction with the same
856+
* read/write and deferrable properties as the local transaction, and start
857+
* the remote subtransaction with the same read/write property as the local
858+
* subtransaction.
846859
*/
847860
static void
848861
begin_remote_xact(ConnCacheEntry *entry)
849862
{
850863
int curlevel = GetCurrentTransactionNestLevel();
851864

852-
/* Start main transaction if we haven't yet */
865+
/*
866+
* Set the nesting level of the topmost read-only transaction if the
867+
* current transaction is read-only and we haven't yet. Once it's set,
868+
* it's retained until that transaction is committed/aborted, and then
869+
* reset (see pgfdw_xact_callback and pgfdw_subxact_callback).
870+
*/
871+
if (XactReadOnly)
872+
{
873+
if (top_read_only_level == 0)
874+
top_read_only_level = GetTopReadOnlyTransactionNestLevel();
875+
Assert(top_read_only_level > 0);
876+
}
877+
else
878+
Assert(top_read_only_level == 0);
879+
880+
/*
881+
* Start main transaction if we haven't yet; otherwise, change the
882+
* already-started remote transaction/subtransaction to read-only if the
883+
* local transaction/subtransaction have been done so after starting them
884+
* and we haven't yet.
885+
*/
853886
if (entry->xact_depth <= 0)
854887
{
855-
const char *sql;
888+
StringInfoData sql;
889+
bool ro = (top_read_only_level == 1);
856890

857891
elog(DEBUG3, "starting remote transaction on connection %p",
858892
entry->conn);
859893

894+
initStringInfo(&sql);
895+
appendStringInfoString(&sql, "START TRANSACTION ISOLATION LEVEL ");
860896
if (IsolationIsSerializable())
861-
sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE";
897+
appendStringInfoString(&sql, "SERIALIZABLE");
862898
else
863-
sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ";
899+
appendStringInfoString(&sql, "REPEATABLE READ");
900+
if (ro)
901+
appendStringInfoString(&sql, " READ ONLY");
902+
if (XactDeferrable)
903+
appendStringInfoString(&sql, " DEFERRABLE");
864904
entry->changing_xact_state = true;
865-
do_sql_command(entry->conn, sql);
905+
do_sql_command(entry->conn, sql.data);
866906
entry->xact_depth = 1;
907+
if (ro)
908+
{
909+
Assert(!entry->xact_read_only);
910+
entry->xact_read_only = true;
911+
}
867912
entry->changing_xact_state = false;
868913
}
914+
else if (!entry->xact_read_only)
915+
{
916+
Assert(top_read_only_level == 0 ||
917+
entry->xact_depth <= top_read_only_level);
918+
if (entry->xact_depth == top_read_only_level)
919+
{
920+
entry->changing_xact_state = true;
921+
do_sql_command(entry->conn, "SET transaction_read_only = on");
922+
entry->xact_read_only = true;
923+
entry->changing_xact_state = false;
924+
}
925+
}
926+
else
927+
Assert(top_read_only_level > 0 &&
928+
entry->xact_depth >= top_read_only_level);
869929

870930
/*
871931
* If we're in a subtransaction, stack up savepoints to match our level.
@@ -874,12 +934,21 @@ begin_remote_xact(ConnCacheEntry *entry)
874934
*/
875935
while (entry->xact_depth < curlevel)
876936
{
877-
char sql[64];
937+
StringInfoData sql;
938+
bool ro = (entry->xact_depth + 1 == top_read_only_level);
878939

879-
snprintf(sql, sizeof(sql), "SAVEPOINT s%d", entry->xact_depth + 1);
940+
initStringInfo(&sql);
941+
appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1);
942+
if (ro)
943+
appendStringInfoString(&sql, "; SET transaction_read_only = on");
880944
entry->changing_xact_state = true;
881-
do_sql_command(entry->conn, sql);
945+
do_sql_command(entry->conn, sql.data);
882946
entry->xact_depth++;
947+
if (ro)
948+
{
949+
Assert(!entry->xact_read_only);
950+
entry->xact_read_only = true;
951+
}
883952
entry->changing_xact_state = false;
884953
}
885954
}
@@ -1174,6 +1243,9 @@ pgfdw_xact_callback(XactEvent event, void *arg)
11741243

11751244
/* Also reset cursor numbering for next transaction */
11761245
cursor_number = 0;
1246+
1247+
/* Likewise for top_read_only_level */
1248+
top_read_only_level = 0;
11771249
}
11781250

11791251
/*
@@ -1272,6 +1344,10 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
12721344
false);
12731345
}
12741346
}
1347+
1348+
/* If in the topmost read-only transaction, reset top_read_only_level */
1349+
if (curlevel == top_read_only_level)
1350+
top_read_only_level = 0;
12751351
}
12761352

12771353
/*
@@ -1374,6 +1450,9 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel)
13741450
/* Reset state to show we're out of a transaction */
13751451
entry->xact_depth = 0;
13761452

1453+
/* Reset xact r/o state */
1454+
entry->xact_read_only = false;
1455+
13771456
/*
13781457
* If the connection isn't in a good idle state, it is marked as
13791458
* invalid or keep_connections option of its server is disabled, then
@@ -1394,6 +1473,10 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel)
13941473
{
13951474
/* Reset state to show we're out of a subtransaction */
13961475
entry->xact_depth--;
1476+
1477+
/* If in the topmost read-only transaction, reset xact r/o state */
1478+
if (entry->xact_depth + 1 == top_read_only_level)
1479+
entry->xact_read_only = false;
13971480
}
13981481
}
13991482

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12384,6 +12384,140 @@ SELECT count(*) FROM remote_application_name
1238412384
DROP FOREIGN TABLE remote_application_name;
1238512385
DROP VIEW my_application_name;
1238612386
-- ===================================================================
12387+
-- test read-only and/or deferrable transactions
12388+
-- ===================================================================
12389+
CREATE TABLE loct (f1 int, f2 text);
12390+
CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS
12391+
'UPDATE public.loct SET f2 = f2 || f2 RETURNING *';
12392+
CREATE VIEW locv AS SELECT t.* FROM locf() t;
12393+
CREATE FOREIGN TABLE remt (f1 int, f2 text)
12394+
SERVER loopback OPTIONS (table_name 'locv');
12395+
CREATE FOREIGN TABLE remt2 (f1 int, f2 text)
12396+
SERVER loopback2 OPTIONS (table_name 'locv');
12397+
INSERT INTO loct VALUES (1, 'foo'), (2, 'bar');
12398+
START TRANSACTION READ ONLY;
12399+
SAVEPOINT s;
12400+
SELECT * FROM remt; -- should fail
12401+
ERROR: cannot execute UPDATE in a read-only transaction
12402+
CONTEXT: SQL function "locf" statement 1
12403+
remote SQL command: SELECT f1, f2 FROM public.locv
12404+
ROLLBACK TO s;
12405+
RELEASE SAVEPOINT s;
12406+
SELECT * FROM remt; -- should fail
12407+
ERROR: cannot execute UPDATE in a read-only transaction
12408+
CONTEXT: SQL function "locf" statement 1
12409+
remote SQL command: SELECT f1, f2 FROM public.locv
12410+
ROLLBACK;
12411+
START TRANSACTION;
12412+
SAVEPOINT s;
12413+
SET transaction_read_only = on;
12414+
SELECT * FROM remt; -- should fail
12415+
ERROR: cannot execute UPDATE in a read-only transaction
12416+
CONTEXT: SQL function "locf" statement 1
12417+
remote SQL command: SELECT f1, f2 FROM public.locv
12418+
ROLLBACK TO s;
12419+
RELEASE SAVEPOINT s;
12420+
SET transaction_read_only = on;
12421+
SELECT * FROM remt; -- should fail
12422+
ERROR: cannot execute UPDATE in a read-only transaction
12423+
CONTEXT: SQL function "locf" statement 1
12424+
remote SQL command: SELECT f1, f2 FROM public.locv
12425+
ROLLBACK;
12426+
START TRANSACTION;
12427+
SAVEPOINT s;
12428+
SELECT * FROM remt; -- should work
12429+
f1 | f2
12430+
----+--------
12431+
1 | foofoo
12432+
2 | barbar
12433+
(2 rows)
12434+
12435+
SET transaction_read_only = on;
12436+
SELECT * FROM remt; -- should fail
12437+
ERROR: cannot execute UPDATE in a read-only transaction
12438+
CONTEXT: SQL function "locf" statement 1
12439+
remote SQL command: SELECT f1, f2 FROM public.locv
12440+
ROLLBACK TO s;
12441+
RELEASE SAVEPOINT s;
12442+
SELECT * FROM remt; -- should work
12443+
f1 | f2
12444+
----+--------
12445+
1 | foofoo
12446+
2 | barbar
12447+
(2 rows)
12448+
12449+
SET transaction_read_only = on;
12450+
SELECT * FROM remt; -- should fail
12451+
ERROR: cannot execute UPDATE in a read-only transaction
12452+
CONTEXT: SQL function "locf" statement 1
12453+
remote SQL command: SELECT f1, f2 FROM public.locv
12454+
ROLLBACK;
12455+
START TRANSACTION;
12456+
SAVEPOINT s;
12457+
SELECT * FROM remt; -- should work
12458+
f1 | f2
12459+
----+--------
12460+
1 | foofoo
12461+
2 | barbar
12462+
(2 rows)
12463+
12464+
SET transaction_read_only = on;
12465+
SELECT * FROM remt2; -- should fail
12466+
ERROR: cannot execute UPDATE in a read-only transaction
12467+
CONTEXT: SQL function "locf" statement 1
12468+
remote SQL command: SELECT f1, f2 FROM public.locv
12469+
ROLLBACK TO s;
12470+
RELEASE SAVEPOINT s;
12471+
SELECT * FROM remt; -- should work
12472+
f1 | f2
12473+
----+--------
12474+
1 | foofoo
12475+
2 | barbar
12476+
(2 rows)
12477+
12478+
SET transaction_read_only = on;
12479+
SELECT * FROM remt2; -- should fail
12480+
ERROR: cannot execute UPDATE in a read-only transaction
12481+
CONTEXT: SQL function "locf" statement 1
12482+
remote SQL command: SELECT f1, f2 FROM public.locv
12483+
ROLLBACK;
12484+
DROP FOREIGN TABLE remt;
12485+
CREATE FOREIGN TABLE remt (f1 int, f2 text)
12486+
SERVER loopback OPTIONS (table_name 'loct');
12487+
START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY;
12488+
SELECT * FROM remt;
12489+
f1 | f2
12490+
----+-----
12491+
1 | foo
12492+
2 | bar
12493+
(2 rows)
12494+
12495+
COMMIT;
12496+
START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE;
12497+
SELECT * FROM remt;
12498+
f1 | f2
12499+
----+-----
12500+
1 | foo
12501+
2 | bar
12502+
(2 rows)
12503+
12504+
COMMIT;
12505+
START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE;
12506+
SELECT * FROM remt;
12507+
f1 | f2
12508+
----+-----
12509+
1 | foo
12510+
2 | bar
12511+
(2 rows)
12512+
12513+
COMMIT;
12514+
-- Clean up
12515+
DROP FOREIGN TABLE remt;
12516+
DROP FOREIGN TABLE remt2;
12517+
DROP VIEW locv;
12518+
DROP FUNCTION locf();
12519+
DROP TABLE loct;
12520+
-- ===================================================================
1238712521
-- test parallel commit and parallel abort
1238812522
-- ===================================================================
1238912523
ALTER SERVER loopback OPTIONS (ADD parallel_commit 'true');

0 commit comments

Comments
 (0)