public inbox for [email protected]help / color / mirror / Atom feed
Options to control remote transactions’ access/deferrable modes in postgres_fdw 20+ messages / 4 participants [nested] [flat]
* Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-03-02 11:44 Etsuro Fujita <[email protected]> 0 siblings, 2 replies; 20+ messages in thread From: Etsuro Fujita @ 2025-03-02 11:44 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, postgres_fdw opens remote transactions in read/write mode in a local transaction even if the local transaction is read-only. I noticed that this leads to surprising behavior like this: CREATE TABLE test (a int); CREATE FUNCTION testfunc() RETURNS int LANGUAGE SQL AS 'INSERT INTO public.test VALUES (1) RETURNING *'; CREATE VIEW testview(a) AS SELECT testfunc(); CREATE FOREIGN TABLE testft (a int) SERVER loopback OPTIONS (table_name 'testview'); START TRANSACTION READ ONLY; SELECT * FROM testft; a --- 1 (1 row) COMMIT; SELECT * FROM test; a --- 1 (1 row) The transaction is declared as READ ONLY, but the INSERT statement is successfully executed in the remote side. To avoid that, I would like to propose a server option, inherit_read_only, to open the remote transactions in read-only mode if the local transaction is read-only. I would also like to propose a server option, inherit_deferrable, to open the remote transactions in deferrable mode if the local transaction is deferrable. Attached is a small patch for these options. I will add this to the March commitfest as it is still open. Best regards, Etsuro Fujita Attachments: [application/octet-stream] Inherit-xact-properties-in-postgres-fdw.patch (5.7K, 2-Inherit-xact-properties-in-postgres-fdw.patch) download | inline diff: diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 8a8d3b4481..c50e86c6a1 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -61,6 +61,8 @@ typedef struct ConnCacheEntry bool have_prep_stmt; /* have we prepared any stmts in this xact? */ bool have_error; /* have any subxacts aborted in this xact? */ bool changing_xact_state; /* xact state change in process */ + bool inherit_read_only; /* do we open xacts in read-only mode? */ + bool inherit_deferrable; /* do we open xacts in dederrable mode? */ bool parallel_commit; /* do we commit (sub)xacts in parallel? */ bool parallel_abort; /* do we abort (sub)xacts in parallel? */ bool invalidated; /* true if reconnect is pending */ @@ -391,6 +393,9 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) * * By default, all the connections to any foreign servers are kept open. * + * Also determine whether to open transactions in read-only and/or + * deferrable modes on the remote server, which is disabled by default. + * * Also determine whether to commit/abort (sub)transactions opened on the * remote server in parallel at (sub)transaction end, which is disabled by * default. @@ -400,6 +405,8 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) * re-made later. */ entry->keep_connections = true; + entry->inherit_read_only = false; + entry->inherit_deferrable = false; entry->parallel_commit = false; entry->parallel_abort = false; foreach(lc, server->options) @@ -408,6 +415,10 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) if (strcmp(def->defname, "keep_connections") == 0) entry->keep_connections = defGetBoolean(def); + else if (strcmp(def->defname, "inherit_read_only") == 0) + entry->inherit_read_only = defGetBoolean(def); + else if (strcmp(def->defname, "inherit_deferrable") == 0) + entry->inherit_deferrable = defGetBoolean(def); else if (strcmp(def->defname, "parallel_commit") == 0) entry->parallel_commit = defGetBoolean(def); else if (strcmp(def->defname, "parallel_abort") == 0) @@ -831,17 +842,26 @@ begin_remote_xact(ConnCacheEntry *entry) /* Start main transaction if we haven't yet */ if (entry->xact_depth <= 0) { - const char *sql; + StringInfoData sql; elog(DEBUG3, "starting remote transaction on connection %p", entry->conn); + initStringInfo(&sql); + appendStringInfoString(&sql, "START TRANSACTION ISOLATION LEVEL "); if (IsolationIsSerializable()) - sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE"; + appendStringInfoString(&sql, "SERIALIZABLE"); else - sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ"; + appendStringInfoString(&sql, "REPEATABLE READ"); + + /* Append access/deferrable modes if needed */ + if (entry->inherit_read_only && XactReadOnly) + appendStringInfoString(&sql, " READ ONLY"); + if (entry->inherit_deferrable && XactDeferrable) + appendStringInfoString(&sql, " DEFERRABLE"); + entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth = 1; entry->changing_xact_state = false; } diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index d0766f007d..e411150318 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -123,6 +123,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) strcmp(def->defname, "updatable") == 0 || strcmp(def->defname, "truncatable") == 0 || strcmp(def->defname, "async_capable") == 0 || + strcmp(def->defname, "inherit_read_only") == 0 || + strcmp(def->defname, "inherit_deferrable") == 0 || strcmp(def->defname, "parallel_commit") == 0 || strcmp(def->defname, "parallel_abort") == 0 || strcmp(def->defname, "keep_connections") == 0) @@ -270,6 +272,8 @@ InitPgFdwOptions(void) /* async_capable is available on both server and table */ {"async_capable", ForeignServerRelationId, false}, {"async_capable", ForeignTableRelationId, false}, + {"inherit_read_only", ForeignServerRelationId, false}, + {"inherit_deferrable", ForeignServerRelationId, false}, {"parallel_commit", ForeignServerRelationId, false}, {"parallel_abort", ForeignServerRelationId, false}, {"keep_connections", ForeignServerRelationId, false}, diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index d2998c13d5..337a2d6252 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -524,6 +524,32 @@ OPTIONS (ADD password_required 'false'); <variablelist> + <varlistentry> + <term><literal>inherit_read_only</literal> (<type>boolean</type>)</term> + <listitem> + <para> + This option controls whether <filename>postgres_fdw</filename> opens + remote transactions in read-only mode on a foreign server in a local + transaction if the local transaction is read-only. + This option can only be specified for foreign servers, not per-table. + The default is <literal>false</literal>. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>inherit_deferrable</literal> (<type>boolean</type>)</term> + <listitem> + <para> + This option controls whether <filename>postgres_fdw</filename> opens + remote transactions in deferrable mode on a foreign server in a local + transaction if the local transaction is deferrable. + This option can only be specified for foreign servers, not per-table. + The default is <literal>false</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>parallel_commit</literal> (<type>boolean</type>)</term> <listitem> ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-03-02 12:01 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 1 sibling, 0 replies; 20+ messages in thread From: Etsuro Fujita @ 2025-03-02 12:01 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> On Sun, Mar 2, 2025 at 12:44 PM Etsuro Fujita <[email protected]> wrote: > Attached is a small patch for these options. I will add this to the > March commitfest as it is still open. The CF was changed to in-progress just before, so I added it to the next CF. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-03-03 12:51 Ashutosh Bapat <[email protected]> parent: Etsuro Fujita <[email protected]> 1 sibling, 2 replies; 20+ messages in thread From: Ashutosh Bapat @ 2025-03-03 12:51 UTC (permalink / raw) To: Etsuro Fujita <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Hi Fujita-san, On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <[email protected]> wrote: > > Hi, > > postgres_fdw opens remote transactions in read/write mode in a local > transaction even if the local transaction is read-only. I noticed > that this leads to surprising behavior like this: > > CREATE TABLE test (a int); > CREATE FUNCTION testfunc() RETURNS int LANGUAGE SQL AS 'INSERT INTO > public.test VALUES (1) RETURNING *'; > CREATE VIEW testview(a) AS SELECT testfunc(); > CREATE FOREIGN TABLE testft (a int) SERVER loopback OPTIONS > (table_name 'testview'); > > START TRANSACTION READ ONLY; > SELECT * FROM testft; > a > --- > 1 > (1 row) > > COMMIT; > SELECT * FROM test; > a > --- > 1 > (1 row) I am having a hard time deciding whether this is problematic behaviour or not. Maybe the way example is setup - it's querying a view on a remote database which doesn't return anything but modified data. If there is no modification happening on the foreign server it won't return any data. Thus we have no way to verify that the table changed because of a READ ONLY transaction which is not expected to change any data. Probably some other example which returns all the rows from test while modifying some of it might be better. > > The transaction is declared as READ ONLY, but the INSERT statement is > successfully executed in the remote side. > > To avoid that, I would like to propose a server option, > inherit_read_only, to open the remote transactions in read-only mode > if the local transaction is read-only. Why do we need a server option. Either we say that a local READ ONLY transaction causing modifications on the foreign server is problematic or it's expected. But what's the point in giving that choice to the user? If we deem the behaviour problematic it should be considered as a bug and we should fix it. Otherwise not fix it. > > I would also like to propose a server option, inherit_deferrable, to > open the remote transactions in deferrable mode if the local > transaction is deferrable. The documentation about deferrable is quite confusing. It says "The DEFERRABLE transaction property has no effect unless the transaction is also SERIALIZABLE and READ ONLY." But it doesn't tell what's the effect of deferrable transaction. But probably we don't need a server option here as well. -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-03-03 15:49 Tom Lane <[email protected]> parent: Ashutosh Bapat <[email protected]> 1 sibling, 1 reply; 20+ messages in thread From: Tom Lane @ 2025-03-03 15:49 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Etsuro Fujita <[email protected]>; PostgreSQL Hackers <[email protected]> Ashutosh Bapat <[email protected]> writes: > On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <[email protected]> wrote: >> To avoid that, I would like to propose a server option, >> inherit_read_only, to open the remote transactions in read-only mode >> if the local transaction is read-only. > Why do we need a server option. Either we say that a local READ ONLY > transaction causing modifications on the foreign server is problematic > or it's expected. But what's the point in giving that choice to the > user? If we deem the behaviour problematic it should be considered as > a bug and we should fix it. Otherwise not fix it. I tend to agree with Ashutosh's position here. Reasoning about issues like this is hard enough already. Having to figure out an application's behavior under more than one setting makes it harder. You may argue that "then the application can choose the behavior it likes, so there's no need to figure out both behaviors". But for a lot of bits of code, that's not the situation; rather, they have to be prepared to work under both settings, because someone else is in charge of what the setting is. (I don't know if either of you recall our disastrous attempt at server-side autocommit, back around 7.3. The reason that got reverted was exactly that there was too much code that had to be prepared to work under either setting, and it was too hard to make that happen. So now I look with great suspicion at anything that complicates our transactional behavior.) >> I would also like to propose a server option, inherit_deferrable, to >> open the remote transactions in deferrable mode if the local >> transaction is deferrable. > The documentation about deferrable is quite confusing. It says "The > DEFERRABLE transaction property has no effect unless the transaction > is also SERIALIZABLE and READ ONLY." But it doesn't tell what's the > effect of deferrable transaction. But probably we don't need a server > option here as well. Yeah, same with this: we should either change it or not. Multiple possible transactional behaviors don't do anyone any favors. regards, tom lane ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-03-25 10:31 Etsuro Fujita <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2025-03-25 10:31 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 3, 2025 at 4:49 PM Tom Lane <[email protected]> wrote: > Ashutosh Bapat <[email protected]> writes: > > On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <[email protected]> wrote: > >> To avoid that, I would like to propose a server option, > >> inherit_read_only, to open the remote transactions in read-only mode > >> if the local transaction is read-only. > > > Why do we need a server option. Either we say that a local READ ONLY > > transaction causing modifications on the foreign server is problematic > > or it's expected. But what's the point in giving that choice to the > > user? If we deem the behaviour problematic it should be considered as > > a bug and we should fix it. Otherwise not fix it. > > I tend to agree with Ashutosh's position here. Reasoning about > issues like this is hard enough already. Having to figure out an > application's behavior under more than one setting makes it harder. > You may argue that "then the application can choose the behavior it > likes, so there's no need to figure out both behaviors". But for a > lot of bits of code, that's not the situation; rather, they have to > be prepared to work under both settings, because someone else is > in charge of what the setting is. (I don't know if either of you > recall our disastrous attempt at server-side autocommit, back around > 7.3. The reason that got reverted was exactly that there was too > much code that had to be prepared to work under either setting, > and it was too hard to make that happen. So now I look with great > suspicion at anything that complicates our transactional behavior.) > > >> I would also like to propose a server option, inherit_deferrable, to > >> open the remote transactions in deferrable mode if the local > >> transaction is deferrable. > > > The documentation about deferrable is quite confusing. It says "The > > DEFERRABLE transaction property has no effect unless the transaction > > is also SERIALIZABLE and READ ONLY." But it doesn't tell what's the > > effect of deferrable transaction. But probably we don't need a server > > option here as well. > > Yeah, same with this: we should either change it or not. Multiple > possible transactional behaviors don't do anyone any favors. I thought some users might rely on the current behavior that remote transactions can write even if the local transaction is READ ONLY, so it would be a good idea to add a server option for backwards compatibility, but I agree that such an option would cause another, more difficult problem you mentioned above. I think the current behavior is not easy to understand, causing unexpected results in a READ ONLY transaction as shown upthread, so I would like to instead propose to fix it (for HEAD only as there are no reports on this issue if I remember correctly). I think those relying on the current behavior should just re-declare their transactions as READ WRITE. Attached is an updated version of the patch, which fixes the deferrable case as well. In the patch I also fixed a bug; I trusted XactReadOnly to see if the local transaction is READ ONLY, but I noticed that that is not 100% correct, because a transaction which started as READ WRITE can show as READ ONLY later within subtransactions, so I modified the patch so that postgres_fdw opens remote transactions in READ ONLY mode if the local transaction has been declared READ ONLY at the top level. Thank you both! Best regards, Etsuro Fujita Attachments: [application/octet-stream] Inherit-xact-properties-in-postgres-fdw-v2.patch (8.2K, 2-Inherit-xact-properties-in-postgres-fdw-v2.patch) download | inline diff: diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 8a8d3b4481..52e262cc18 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -822,6 +822,9 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input) * those scans. A disadvantage is that we can't provide sane emulation of * READ COMMITTED behavior --- it would be nice if we had some other way to * control which remote queries share a snapshot. + * + * Note that we always start remote transactions with the same read/write + * and deferrable properties as the local (top-level) transaction. */ static void begin_remote_xact(ConnCacheEntry *entry) @@ -831,17 +834,23 @@ begin_remote_xact(ConnCacheEntry *entry) /* Start main transaction if we haven't yet */ if (entry->xact_depth <= 0) { - const char *sql; + StringInfoData sql; elog(DEBUG3, "starting remote transaction on connection %p", entry->conn); + initStringInfo(&sql); + appendStringInfoString(&sql, "START TRANSACTION ISOLATION LEVEL "); if (IsolationIsSerializable()) - sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE"; + appendStringInfoString(&sql, "SERIALIZABLE"); else - sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ"; + appendStringInfoString(&sql, "REPEATABLE READ"); + if (TopTransactionIsReadOnly()) + appendStringInfoString(&sql, " READ ONLY"); + if (XactDeferrable) + appendStringInfoString(&sql, " DEFERRABLE"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth = 1; entry->changing_xact_state = false; } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 8447b289cb..ba3bb7665a 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -12263,6 +12263,78 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; -- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); +SELECT * FROM loct; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SELECT * FROM loct; + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +START TRANSACTION READ ONLY; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +COMMIT; +-- Clean up +DROP FOREIGN TABLE remt; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; +-- =================================================================== -- test parallel commit and parallel abort -- =================================================================== ALTER SERVER loopback OPTIONS (ADD parallel_commit 'true'); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 1598d9e086..292281981c 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -4165,6 +4165,48 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; +-- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); + +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); +SELECT * FROM loct; + +SELECT * FROM remt; -- should work +SELECT * FROM loct; + +START TRANSACTION READ ONLY; +SELECT * FROM remt; -- should fail +ROLLBACK; + +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +-- Clean up +DROP FOREIGN TABLE remt; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; + -- =================================================================== -- test parallel commit and parallel abort -- =================================================================== diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index d2998c13d5..13d1944ab5 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -1071,6 +1071,22 @@ postgres=# SELECT postgres_fdw_disconnect_all(); <productname>PostgreSQL</productname> release might modify these rules. </para> + <para> + The remote transaction is opened in the same read/write mode as the local + transaction: if the local transaction has been declared + <literal>READ ONLY</literal> at the top level, the remote transaction is + opened in <literal>READ ONLY</literal> mode, otherwise it is opened in + <literal>READ WRITE</literal> mode. + </para> + + <para> + The remote transaction is also opened in the same deferrable mode as the + local transaction: if the local transaction has been declared + <literal>DEFERRABLE</literal> at the top level, the remote transaction is + opened in <literal>DEFERRABLE</literal> mode, otherwise it is opened in + <literal>NOT DEFERRABLE</literal> mode. + </para> + <para> Note that it is currently not supported by <filename>postgres_fdw</filename> to prepare the remote transaction for diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 1b4f21a88d..3e79922c8f 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1043,6 +1043,27 @@ TransactionStartedDuringRecovery(void) return CurrentTransactionState->startedInRecovery; } +/* + * TopTransactionIsReadOnly + * + * Returns true if the transaction has been declared READ ONLY at the top + * level. + */ +bool +TopTransactionIsReadOnly(void) +{ + TransactionState s = CurrentTransactionState; + + if (s->nestingLevel == 1) + return XactReadOnly; + while (s->nestingLevel > 2) + { + Assert(s->parent != NULL); + s = s->parent; + } + return s->prevXactReadOnly; +} + /* * EnterParallelMode */ diff --git a/src/include/access/xact.h b/src/include/access/xact.h index b2bc10ee04..0160eb1d91 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -458,6 +458,7 @@ extern TimestampTz GetCurrentTransactionStopTimestamp(void); extern void SetCurrentStatementStartTimestamp(void); extern int GetCurrentTransactionNestLevel(void); extern bool TransactionIdIsCurrentTransactionId(TransactionId xid); +extern bool TopTransactionIsReadOnly(void); extern void CommandCounterIncrement(void); extern void ForceSyncCommit(void); extern void StartTransactionCommand(void); ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-03-25 10:44 Etsuro Fujita <[email protected]> parent: Ashutosh Bapat <[email protected]> 1 sibling, 0 replies; 20+ messages in thread From: Etsuro Fujita @ 2025-03-25 10:44 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, Mar 3, 2025 at 1:51 PM Ashutosh Bapat <[email protected]> wrote: > On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <[email protected]> wrote: > > postgres_fdw opens remote transactions in read/write mode in a local > > transaction even if the local transaction is read-only. I noticed > > that this leads to surprising behavior like this: > I am having a hard time deciding whether this is problematic behaviour > or not. Maybe the way example is setup - it's querying a view on a > remote database which doesn't return anything but modified data. If > there is no modification happening on the foreign server it won't > return any data. Thus we have no way to verify that the table changed > because of a READ ONLY transaction which is not expected to change any > data. Probably some other example which returns all the rows from test > while modifying some of it might be better. How about something like this? CREATE TABLE loct (f1 int, f2 text); CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; CREATE VIEW locv AS SELECT t.* FROM locf() t; CREATE FOREIGN TABLE remt (f1 int, f2 text) SERVER loopback OPTIONS (table_name 'locv'); INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); SELECT * FROM loct; f1 | f2 ----+----- 1 | foo 2 | bar (2 rows) SELECT * FROM remt; -- should work f1 | f2 ----+-------- 1 | foofoo 2 | barbar (2 rows) SELECT * FROM loct; f1 | f2 ----+-------- 1 | foofoo 2 | barbar (2 rows) I added this test case to the updated patch [1]. Thanks for the comments! Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/CAPmGK14Btk0odkH6vwBhBGjCexmmWcM_D3DG0pJtObj8k_Unag%40mail.gma... ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-03-27 12:25 Ashutosh Bapat <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Ashutosh Bapat @ 2025-03-27 12:25 UTC (permalink / raw) To: Etsuro Fujita <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 25, 2025 at 4:01 PM Etsuro Fujita <[email protected]> wrote: > > In the patch I also fixed a bug; I trusted XactReadOnly to see if the > local transaction is READ ONLY, but I noticed that that is not 100% > correct, because a transaction which started as READ WRITE can show as > READ ONLY later within subtransactions, so I modified the patch so > that postgres_fdw opens remote transactions in READ ONLY mode if the > local transaction has been declared READ ONLY at the top level. Nice catch. postgres_fdw replicates the transaction stack on foreign server. I think we need to replicate it along with the transaction properties. And also we need a test which tests readonly subtransaction behaviour. -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-03-30 10:14 Etsuro Fujita <[email protected]> parent: Ashutosh Bapat <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2025-03-30 10:14 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 27, 2025 at 1:25 PM Ashutosh Bapat <[email protected]> wrote: > On Tue, Mar 25, 2025 at 4:01 PM Etsuro Fujita <[email protected]> wrote: > > In the patch I also fixed a bug; I trusted XactReadOnly to see if the > > local transaction is READ ONLY, but I noticed that that is not 100% > > correct, because a transaction which started as READ WRITE can show as > > READ ONLY later within subtransactions, so I modified the patch so > > that postgres_fdw opens remote transactions in READ ONLY mode if the > > local transaction has been declared READ ONLY at the top level. > > Nice catch. postgres_fdw replicates the transaction stack on foreign > server. I think we need to replicate it along with the transaction > properties. And also we need a test which tests readonly > subtransaction behaviour. Ok, will do. Thanks for the comment! Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-05-04 12:30 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2025-05-04 12:30 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Sun, Mar 30, 2025 at 7:14 PM Etsuro Fujita <[email protected]> wrote: > On Thu, Mar 27, 2025 at 1:25 PM Ashutosh Bapat > <[email protected]> wrote: > > On Tue, Mar 25, 2025 at 4:01 PM Etsuro Fujita <[email protected]> wrote: > > > In the patch I also fixed a bug; I trusted XactReadOnly to see if the > > > local transaction is READ ONLY, but I noticed that that is not 100% > > > correct, because a transaction which started as READ WRITE can show as > > > READ ONLY later within subtransactions, so I modified the patch so > > > that postgres_fdw opens remote transactions in READ ONLY mode if the > > > local transaction has been declared READ ONLY at the top level. > > > > Nice catch. postgres_fdw replicates the transaction stack on foreign > > server. I think we need to replicate it along with the transaction > > properties. And also we need a test which tests readonly > > subtransaction behaviour. > > Ok, will do. I noticed that we don’t need to replicate it. As read-only subtransactions can’t change to read-write, and a read-only main-transaction can’t change to read-write after first snapshot, either (note: begin_remote_xact is called after it), all we need to do is track the nesting level of the outermost read-only transaction in the local transaction and control the access mode of remote transactions based on it so that they have the same access mode as the local transaction at each subtransaction level, like the attached. This makes the patch pretty simple. I added the tests in the patch. What do you think about that? Best regards, Etsuro Fujita Attachments: [application/octet-stream] Inherit-xact-properties-in-postgres-fdw-v3.patch (14.7K, 2-Inherit-xact-properties-in-postgres-fdw-v3.patch) download | inline diff: diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 9fa7f7e95cd..67afab2e456 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -58,6 +58,7 @@ typedef struct ConnCacheEntry /* Remaining fields are invalid when conn is NULL: */ int xact_depth; /* 0 = no xact open, 1 = main xact open, 2 = * one level of subxact open, etc */ + bool xact_read_only; /* xact r/o state */ bool have_prep_stmt; /* have we prepared any stmts in this xact? */ bool have_error; /* have any subxacts aborted in this xact? */ bool changing_xact_state; /* xact state change in process */ @@ -84,6 +85,12 @@ static unsigned int prep_stmt_number = 0; /* tracks whether any work is needed in callback functions */ static bool xact_got_connection = false; +/* + * tracks the nesting level of the topmost read-only transaction determined + * by GetTopReadOnlyTransactionNestLevel() + */ +static int top_read_only_level = 0; + /* custom wait event values, retrieved from shared memory */ static uint32 pgfdw_we_cleanup_result = 0; static uint32 pgfdw_we_connect = 0; @@ -372,6 +379,7 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) /* Reset all transient state fields, to be sure all are clean */ entry->xact_depth = 0; + entry->xact_read_only = false; entry->have_prep_stmt = false; entry->have_error = false; entry->changing_xact_state = false; @@ -843,29 +851,80 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input) * those scans. A disadvantage is that we can't provide sane emulation of * READ COMMITTED behavior --- it would be nice if we had some other way to * control which remote queries share a snapshot. + * + * Note also that we always start the remote transaction with the same + * read/write and deferrable properties as the local transaction, and start + * the remote subtransaction with the same read/write property as the local + * subtransaction. */ static void begin_remote_xact(ConnCacheEntry *entry) { int curlevel = GetCurrentTransactionNestLevel(); - /* Start main transaction if we haven't yet */ + /* + * Set the nesting level of the topmost read-only transaction if the + * current transaction is read-only and we haven't yet. Once it's set, + * it's retained until that transaction is committed/aborted. + */ + if (XactReadOnly) + { + if (top_read_only_level == 0) + top_read_only_level = GetTopReadOnlyTransactionNestLevel(); + Assert(top_read_only_level > 0); + } + else + Assert(top_read_only_level == 0); + + /* + * Start main transaction if we haven't yet; otherwise, change the + * already-started remote transaction/subtransaction to read-only if the + * local transaction/subtransaction have been done so after starting them + * and we haven't yet. + */ if (entry->xact_depth <= 0) { - const char *sql; + StringInfoData sql; + bool ro = (top_read_only_level == 1); elog(DEBUG3, "starting remote transaction on connection %p", entry->conn); + initStringInfo(&sql); + appendStringInfoString(&sql, "START TRANSACTION ISOLATION LEVEL "); if (IsolationIsSerializable()) - sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE"; + appendStringInfoString(&sql, "SERIALIZABLE"); else - sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ"; + appendStringInfoString(&sql, "REPEATABLE READ"); + if (ro) + appendStringInfoString(&sql, " READ ONLY"); + if (XactDeferrable) + appendStringInfoString(&sql, " DEFERRABLE"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth = 1; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } + else if (!entry->xact_read_only) + { + Assert(top_read_only_level == 0 || + entry->xact_depth <= top_read_only_level); + if (entry->xact_depth == top_read_only_level) + { + entry->changing_xact_state = true; + do_sql_command(entry->conn, "SET transaction_read_only = on"); + entry->xact_read_only = true; + entry->changing_xact_state = false; + } + } + else + Assert(top_read_only_level > 0 && + entry->xact_depth >= top_read_only_level); /* * If we're in a subtransaction, stack up savepoints to match our level. @@ -874,12 +933,21 @@ begin_remote_xact(ConnCacheEntry *entry) */ while (entry->xact_depth < curlevel) { - char sql[64]; + StringInfoData sql; + bool ro = (entry->xact_depth + 1 == top_read_only_level); - snprintf(sql, sizeof(sql), "SAVEPOINT s%d", entry->xact_depth + 1); + initStringInfo(&sql); + appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1); + if (ro) + appendStringInfoString(&sql, "; SET transaction_read_only = on"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth++; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } } @@ -1174,6 +1242,9 @@ pgfdw_xact_callback(XactEvent event, void *arg) /* Also reset cursor numbering for next transaction */ cursor_number = 0; + + /* Likewise for top_read_only_level */ + top_read_only_level = 0; } /* @@ -1272,6 +1343,10 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid, false); } } + + /* If in the topmost read-only transaction, reset top_read_only_level */ + if (curlevel == top_read_only_level) + top_read_only_level = 0; } /* @@ -1374,6 +1449,9 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) /* Reset state to show we're out of a transaction */ entry->xact_depth = 0; + /* Reset xact r/o state */ + entry->xact_read_only = false; + /* * If the connection isn't in a good idle state, it is marked as * invalid or keep_connections option of its server is disabled, then @@ -1394,6 +1472,13 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) { /* Reset state to show we're out of a subtransaction */ entry->xact_depth--; + + /* If in the topmost read-only transaction, reset xact r/o state */ + if (entry->xact_depth + 1 == top_read_only_level) + { + Assert(entry->xact_read_only); + entry->xact_read_only = false; + } } } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 24ff5f70cce..e5d44da9fb5 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -12320,6 +12320,108 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; -- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +-- Clean up +DROP FOREIGN TABLE remt; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; +-- =================================================================== -- test parallel commit and parallel abort -- =================================================================== ALTER SERVER loopback OPTIONS (ADD parallel_commit 'true'); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 1f27260bafe..32734931be5 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -4195,6 +4195,69 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; +-- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); + +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +-- Clean up +DROP FOREIGN TABLE remt; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; + -- =================================================================== -- test parallel commit and parallel abort -- =================================================================== diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 781a01067f7..4c8ee383c63 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -1077,6 +1077,21 @@ postgres=# SELECT postgres_fdw_disconnect_all(); <productname>PostgreSQL</productname> release might modify these rules. </para> + <para> + The remote transaction is opened in the same read/write mode as the local + transaction: if the local transaction is <literal>READ ONLY</literal>, + the remote transaction is opened in <literal>READ ONLY</literal> mode, + otherwise it is opened in <literal>READ WRITE</literal> mode. + (This control is also applied to remote and local subtransactions.) + </para> + + <para> + The remote transaction is also opened in the same deferrable mode as the + local transaction: if the local transaction is <literal>DEFERRABLE</literal>, + the remote transaction is opened in <literal>DEFERRABLE</literal> mode, + otherwise it is opened in <literal>NOT DEFERRABLE</literal> mode. + </para> + <para> Note that it is currently not supported by <filename>postgres_fdw</filename> to prepare the remote transaction for diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b885513f765..294d1eea218 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1044,6 +1044,30 @@ TransactionStartedDuringRecovery(void) return CurrentTransactionState->startedInRecovery; } +/* + * GetTopReadOnlyTransactionNestLevel + * + * Note: this will return zero when not inside any transaction or when neither + * a top-level transaction nor subtransactions are read-only, one when the + * top-level transaction is read-only, two when one level of subtransaction is + * read-only, etc. + */ +int +GetTopReadOnlyTransactionNestLevel(void) +{ + TransactionState s = CurrentTransactionState; + + if (!XactReadOnly) + return 0; + while (s->nestingLevel > 1) + { + if (!s->prevXactReadOnly) + return s->nestingLevel; + s = s->parent; + } + return s->nestingLevel; +} + /* * EnterParallelMode */ diff --git a/src/include/access/xact.h b/src/include/access/xact.h index b2bc10ee041..7f11b919799 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -458,6 +458,7 @@ extern TimestampTz GetCurrentTransactionStopTimestamp(void); extern void SetCurrentStatementStartTimestamp(void); extern int GetCurrentTransactionNestLevel(void); extern bool TransactionIdIsCurrentTransactionId(TransactionId xid); +extern int GetTopReadOnlyTransactionNestLevel(void); extern void CommandCounterIncrement(void); extern void ForceSyncCommit(void); extern void StartTransactionCommand(void); ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-05-08 08:50 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2025-05-08 08:50 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Sun, May 4, 2025 at 9:30 PM Etsuro Fujita <[email protected]> wrote: > As read-only > subtransactions can’t change to read-write, and a read-only > main-transaction can’t change to read-write after first snapshot, > either (note: begin_remote_xact is called after it), all we need to do > is track the nesting level of the outermost read-only transaction in > the local transaction and control the access mode of remote > transactions based on it so that they have the same access mode as the > local transaction at each subtransaction level, like the attached. I noticed that an assertion I added to pgfdw_subxact_callback fails when running some transactions. To fix, I just removed the assertion. Attached is an updated version of the patch. I also added a test case that was causing the failure, and tweaked some comments/docs a little bit. Best regards, Etsuro Fujita Attachments: [application/octet-stream] Inherit-xact-properties-in-postgres-fdw-v4.patch (16.0K, 2-Inherit-xact-properties-in-postgres-fdw-v4.patch) download | inline diff: diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 9fa7f7e95cd..fe86bf53a37 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -58,6 +58,7 @@ typedef struct ConnCacheEntry /* Remaining fields are invalid when conn is NULL: */ int xact_depth; /* 0 = no xact open, 1 = main xact open, 2 = * one level of subxact open, etc */ + bool xact_read_only; /* xact r/o state */ bool have_prep_stmt; /* have we prepared any stmts in this xact? */ bool have_error; /* have any subxacts aborted in this xact? */ bool changing_xact_state; /* xact state change in process */ @@ -84,6 +85,12 @@ static unsigned int prep_stmt_number = 0; /* tracks whether any work is needed in callback functions */ static bool xact_got_connection = false; +/* + * tracks the nesting level of the topmost read-only transaction determined + * by GetTopReadOnlyTransactionNestLevel() + */ +static int top_read_only_level = 0; + /* custom wait event values, retrieved from shared memory */ static uint32 pgfdw_we_cleanup_result = 0; static uint32 pgfdw_we_connect = 0; @@ -372,6 +379,7 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) /* Reset all transient state fields, to be sure all are clean */ entry->xact_depth = 0; + entry->xact_read_only = false; entry->have_prep_stmt = false; entry->have_error = false; entry->changing_xact_state = false; @@ -843,29 +851,81 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input) * those scans. A disadvantage is that we can't provide sane emulation of * READ COMMITTED behavior --- it would be nice if we had some other way to * control which remote queries share a snapshot. + * + * Note also that we always start the remote transaction with the same + * read/write and deferrable properties as the local transaction, and start + * the remote subtransaction with the same read/write property as the local + * subtransaction. */ static void begin_remote_xact(ConnCacheEntry *entry) { int curlevel = GetCurrentTransactionNestLevel(); - /* Start main transaction if we haven't yet */ + /* + * Set the nesting level of the topmost read-only transaction if the + * current transaction is read-only and we haven't yet. Once it's set, + * it's retained until that transaction is committed/aborted, and then + * reset (see pgfdw_xact_callback and pgfdw_subxact_callback). + */ + if (XactReadOnly) + { + if (top_read_only_level == 0) + top_read_only_level = GetTopReadOnlyTransactionNestLevel(); + Assert(top_read_only_level > 0); + } + else + Assert(top_read_only_level == 0); + + /* + * Start main transaction if we haven't yet; otherwise, change the + * already-started remote transaction/subtransaction to read-only if the + * local transaction/subtransaction have been done so after starting them + * and we haven't yet. + */ if (entry->xact_depth <= 0) { - const char *sql; + StringInfoData sql; + bool ro = (top_read_only_level == 1); elog(DEBUG3, "starting remote transaction on connection %p", entry->conn); + initStringInfo(&sql); + appendStringInfoString(&sql, "START TRANSACTION ISOLATION LEVEL "); if (IsolationIsSerializable()) - sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE"; + appendStringInfoString(&sql, "SERIALIZABLE"); else - sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ"; + appendStringInfoString(&sql, "REPEATABLE READ"); + if (ro) + appendStringInfoString(&sql, " READ ONLY"); + if (XactDeferrable) + appendStringInfoString(&sql, " DEFERRABLE"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth = 1; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } + else if (!entry->xact_read_only) + { + Assert(top_read_only_level == 0 || + entry->xact_depth <= top_read_only_level); + if (entry->xact_depth == top_read_only_level) + { + entry->changing_xact_state = true; + do_sql_command(entry->conn, "SET transaction_read_only = on"); + entry->xact_read_only = true; + entry->changing_xact_state = false; + } + } + else + Assert(top_read_only_level > 0 && + entry->xact_depth >= top_read_only_level); /* * If we're in a subtransaction, stack up savepoints to match our level. @@ -874,12 +934,21 @@ begin_remote_xact(ConnCacheEntry *entry) */ while (entry->xact_depth < curlevel) { - char sql[64]; + StringInfoData sql; + bool ro = (entry->xact_depth + 1 == top_read_only_level); - snprintf(sql, sizeof(sql), "SAVEPOINT s%d", entry->xact_depth + 1); + initStringInfo(&sql); + appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1); + if (ro) + appendStringInfoString(&sql, "; SET transaction_read_only = on"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth++; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } } @@ -1174,6 +1243,9 @@ pgfdw_xact_callback(XactEvent event, void *arg) /* Also reset cursor numbering for next transaction */ cursor_number = 0; + + /* Likewise for top_read_only_level */ + top_read_only_level = 0; } /* @@ -1272,6 +1344,10 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid, false); } } + + /* If in the topmost read-only transaction, reset top_read_only_level */ + if (curlevel == top_read_only_level) + top_read_only_level = 0; } /* @@ -1374,6 +1450,9 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) /* Reset state to show we're out of a transaction */ entry->xact_depth = 0; + /* Reset xact r/o state */ + entry->xact_read_only = false; + /* * If the connection isn't in a good idle state, it is marked as * invalid or keep_connections option of its server is disabled, then @@ -1394,6 +1473,10 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) { /* Reset state to show we're out of a subtransaction */ entry->xact_depth--; + + /* If in the topmost read-only transaction, reset xact r/o state */ + if (entry->xact_depth + 1 == top_read_only_level) + entry->xact_read_only = false; } } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 24ff5f70cce..969d357ef08 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -12320,6 +12320,140 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; -- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +CREATE FOREIGN TABLE remt2 (f1 int, f2 text) + SERVER loopback2 OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +-- Clean up +DROP FOREIGN TABLE remt; +DROP FOREIGN TABLE remt2; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; +-- =================================================================== -- test parallel commit and parallel abort -- =================================================================== ALTER SERVER loopback OPTIONS (ADD parallel_commit 'true'); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 1f27260bafe..099c6cde4b8 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -4195,6 +4195,84 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; +-- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +CREATE FOREIGN TABLE remt2 (f1 int, f2 text) + SERVER loopback2 OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); + +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ROLLBACK; + +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +-- Clean up +DROP FOREIGN TABLE remt; +DROP FOREIGN TABLE remt2; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; + -- =================================================================== -- test parallel commit and parallel abort -- =================================================================== diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 781a01067f7..c464716e3ce 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -1077,6 +1077,21 @@ postgres=# SELECT postgres_fdw_disconnect_all(); <productname>PostgreSQL</productname> release might modify these rules. </para> + <para> + The remote transaction is opened in the same read/write mode as the local + transaction: if the local transaction is <literal>READ ONLY</literal>, + the remote transaction is opened in <literal>READ ONLY</literal> mode, + otherwise it is opened in <literal>READ WRITE</literal> mode. + (This rule is also applied to remote and local subtransactions.) + </para> + + <para> + The remote transaction is also opened in the same deferrable mode as the + local transaction: if the local transaction is <literal>DEFERRABLE</literal>, + the remote transaction is opened in <literal>DEFERRABLE</literal> mode, + otherwise it is opened in <literal>NOT DEFERRABLE</literal> mode. + </para> + <para> Note that it is currently not supported by <filename>postgres_fdw</filename> to prepare the remote transaction for diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b885513f765..294d1eea218 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1044,6 +1044,30 @@ TransactionStartedDuringRecovery(void) return CurrentTransactionState->startedInRecovery; } +/* + * GetTopReadOnlyTransactionNestLevel + * + * Note: this will return zero when not inside any transaction or when neither + * a top-level transaction nor subtransactions are read-only, one when the + * top-level transaction is read-only, two when one level of subtransaction is + * read-only, etc. + */ +int +GetTopReadOnlyTransactionNestLevel(void) +{ + TransactionState s = CurrentTransactionState; + + if (!XactReadOnly) + return 0; + while (s->nestingLevel > 1) + { + if (!s->prevXactReadOnly) + return s->nestingLevel; + s = s->parent; + } + return s->nestingLevel; +} + /* * EnterParallelMode */ diff --git a/src/include/access/xact.h b/src/include/access/xact.h index b2bc10ee041..7f11b919799 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -458,6 +458,7 @@ extern TimestampTz GetCurrentTransactionStopTimestamp(void); extern void SetCurrentStatementStartTimestamp(void); extern int GetCurrentTransactionNestLevel(void); extern bool TransactionIdIsCurrentTransactionId(TransactionId xid); +extern int GetTopReadOnlyTransactionNestLevel(void); extern void CommandCounterIncrement(void); extern void ForceSyncCommit(void); extern void StartTransactionCommand(void); ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-05-25 05:39 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2025-05-25 05:39 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, May 8, 2025 at 5:50 PM Etsuro Fujita <[email protected]> wrote: > Attached is an updated version of the patch. Here is a new version of the patch where I added a comment for a new function, fixed indentation, and added the commit message. If there are no objections, I will push this as a master-only fix, as noted in the commit message. Best regards, Etsuro Fujita Attachments: [application/octet-stream] v5-0001-postgres_fdw-Inherit-the-local-transaction-s-access-.patch (18.0K, 2-v5-0001-postgres_fdw-Inherit-the-local-transaction-s-access-.patch) download | inline diff: From 30897a4bff6c6d32299bdd2d2b1413f2d2b2d255 Mon Sep 17 00:00:00 2001 From: Etsuro Fujita <[email protected]> Date: Sun, 25 May 2025 14:19:15 +0900 Subject: [PATCH] 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 <[email protected]> Reviewed-by: Ashutosh Bapat <[email protected]> Reviewed-by: Tom Lane <[email protected]> Discussion: https://postgr.es/m/CAPmGK16n_hcUUWuOdmeUS%2Bw4Q6dZvTEDHb%3DOP%3D5JBzo-M3QmpQ%40mail.gmail.com --- contrib/postgres_fdw/connection.c | 99 +++++++++++-- .../postgres_fdw/expected/postgres_fdw.out | 134 ++++++++++++++++++ contrib/postgres_fdw/sql/postgres_fdw.sql | 78 ++++++++++ doc/src/sgml/postgres-fdw.sgml | 15 ++ src/backend/access/transam/xact.c | 28 ++++ src/include/access/xact.h | 1 + 6 files changed, 347 insertions(+), 8 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 304f3c20f83..caf14462696 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -58,6 +58,7 @@ typedef struct ConnCacheEntry /* Remaining fields are invalid when conn is NULL: */ int xact_depth; /* 0 = no xact open, 1 = main xact open, 2 = * one level of subxact open, etc */ + bool xact_read_only; /* xact r/o state */ bool have_prep_stmt; /* have we prepared any stmts in this xact? */ bool have_error; /* have any subxacts aborted in this xact? */ bool changing_xact_state; /* xact state change in process */ @@ -84,6 +85,12 @@ static unsigned int prep_stmt_number = 0; /* tracks whether any work is needed in callback functions */ static bool xact_got_connection = false; +/* + * tracks the nesting level of the topmost read-only transaction determined + * by GetTopReadOnlyTransactionNestLevel() + */ +static int top_read_only_level = 0; + /* custom wait event values, retrieved from shared memory */ static uint32 pgfdw_we_cleanup_result = 0; static uint32 pgfdw_we_connect = 0; @@ -372,6 +379,7 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) /* Reset all transient state fields, to be sure all are clean */ entry->xact_depth = 0; + entry->xact_read_only = false; entry->have_prep_stmt = false; entry->have_error = false; entry->changing_xact_state = false; @@ -843,29 +851,81 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input) * those scans. A disadvantage is that we can't provide sane emulation of * READ COMMITTED behavior --- it would be nice if we had some other way to * control which remote queries share a snapshot. + * + * Note also that we always start the remote transaction with the same + * read/write and deferrable properties as the local transaction, and start + * the remote subtransaction with the same read/write property as the local + * subtransaction. */ static void begin_remote_xact(ConnCacheEntry *entry) { int curlevel = GetCurrentTransactionNestLevel(); - /* Start main transaction if we haven't yet */ + /* + * Set the nesting level of the topmost read-only transaction if the + * current transaction is read-only and we haven't yet. Once it's set, + * it's retained until that transaction is committed/aborted, and then + * reset (see pgfdw_xact_callback and pgfdw_subxact_callback). + */ + if (XactReadOnly) + { + if (top_read_only_level == 0) + top_read_only_level = GetTopReadOnlyTransactionNestLevel(); + Assert(top_read_only_level > 0); + } + else + Assert(top_read_only_level == 0); + + /* + * Start main transaction if we haven't yet; otherwise, change the + * already-started remote transaction/subtransaction to read-only if the + * local transaction/subtransaction have been done so after starting them + * and we haven't yet. + */ if (entry->xact_depth <= 0) { - const char *sql; + StringInfoData sql; + bool ro = (top_read_only_level == 1); elog(DEBUG3, "starting remote transaction on connection %p", entry->conn); + initStringInfo(&sql); + appendStringInfoString(&sql, "START TRANSACTION ISOLATION LEVEL "); if (IsolationIsSerializable()) - sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE"; + appendStringInfoString(&sql, "SERIALIZABLE"); else - sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ"; + appendStringInfoString(&sql, "REPEATABLE READ"); + if (ro) + appendStringInfoString(&sql, " READ ONLY"); + if (XactDeferrable) + appendStringInfoString(&sql, " DEFERRABLE"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth = 1; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } + else if (!entry->xact_read_only) + { + Assert(top_read_only_level == 0 || + entry->xact_depth <= top_read_only_level); + if (entry->xact_depth == top_read_only_level) + { + entry->changing_xact_state = true; + do_sql_command(entry->conn, "SET transaction_read_only = on"); + entry->xact_read_only = true; + entry->changing_xact_state = false; + } + } + else + Assert(top_read_only_level > 0 && + entry->xact_depth >= top_read_only_level); /* * If we're in a subtransaction, stack up savepoints to match our level. @@ -874,12 +934,21 @@ begin_remote_xact(ConnCacheEntry *entry) */ while (entry->xact_depth < curlevel) { - char sql[64]; + StringInfoData sql; + bool ro = (entry->xact_depth + 1 == top_read_only_level); - snprintf(sql, sizeof(sql), "SAVEPOINT s%d", entry->xact_depth + 1); + initStringInfo(&sql); + appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1); + if (ro) + appendStringInfoString(&sql, "; SET transaction_read_only = on"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth++; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } } @@ -1174,6 +1243,9 @@ pgfdw_xact_callback(XactEvent event, void *arg) /* Also reset cursor numbering for next transaction */ cursor_number = 0; + + /* Likewise for top_read_only_level */ + top_read_only_level = 0; } /* @@ -1272,6 +1344,10 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid, false); } } + + /* If in the topmost read-only transaction, reset top_read_only_level */ + if (curlevel == top_read_only_level) + top_read_only_level = 0; } /* @@ -1374,6 +1450,9 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) /* Reset state to show we're out of a transaction */ entry->xact_depth = 0; + /* Reset xact r/o state */ + entry->xact_read_only = false; + /* * If the connection isn't in a good idle state, it is marked as * invalid or keep_connections option of its server is disabled, then @@ -1394,6 +1473,10 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) { /* Reset state to show we're out of a subtransaction */ entry->xact_depth--; + + /* If in the topmost read-only transaction, reset xact r/o state */ + if (entry->xact_depth + 1 == top_read_only_level) + entry->xact_read_only = false; } } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 2185b42bb4f..eb4716bed81 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -12384,6 +12384,140 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; -- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +CREATE FOREIGN TABLE remt2 (f1 int, f2 text) + SERVER loopback2 OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +-- Clean up +DROP FOREIGN TABLE remt; +DROP FOREIGN TABLE remt2; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; +-- =================================================================== -- test parallel commit and parallel abort -- =================================================================== ALTER SERVER loopback OPTIONS (ADD parallel_commit 'true'); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index e534b40de3c..20a535b99d8 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -4200,6 +4200,84 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; +-- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +CREATE FOREIGN TABLE remt2 (f1 int, f2 text) + SERVER loopback2 OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); + +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ROLLBACK; + +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +-- Clean up +DROP FOREIGN TABLE remt; +DROP FOREIGN TABLE remt2; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; + -- =================================================================== -- test parallel commit and parallel abort -- =================================================================== diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 781a01067f7..c464716e3ce 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -1077,6 +1077,21 @@ postgres=# SELECT postgres_fdw_disconnect_all(); <productname>PostgreSQL</productname> release might modify these rules. </para> + <para> + The remote transaction is opened in the same read/write mode as the local + transaction: if the local transaction is <literal>READ ONLY</literal>, + the remote transaction is opened in <literal>READ ONLY</literal> mode, + otherwise it is opened in <literal>READ WRITE</literal> mode. + (This rule is also applied to remote and local subtransactions.) + </para> + + <para> + The remote transaction is also opened in the same deferrable mode as the + local transaction: if the local transaction is <literal>DEFERRABLE</literal>, + the remote transaction is opened in <literal>DEFERRABLE</literal> mode, + otherwise it is opened in <literal>NOT DEFERRABLE</literal> mode. + </para> + <para> Note that it is currently not supported by <filename>postgres_fdw</filename> to prepare the remote transaction for diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b885513f765..df10c538bba 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1044,6 +1044,34 @@ TransactionStartedDuringRecovery(void) return CurrentTransactionState->startedInRecovery; } +/* + * GetTopReadOnlyTransactionNestLevel + * + * Note: this will return zero when not inside any transaction or when neither + * a top-level transaction nor subtransactions are read-only, one when the + * top-level transaction is read-only, two when one level of subtransaction is + * read-only, etc. + * + * Note: subtransactions of the topmost read-only transaction are read-only, + * because they inherit read-only mode from the parent transaction and can't + * go to read-write mode. + */ +int +GetTopReadOnlyTransactionNestLevel(void) +{ + TransactionState s = CurrentTransactionState; + + if (!XactReadOnly) + return 0; + while (s->nestingLevel > 1) + { + if (!s->prevXactReadOnly) + return s->nestingLevel; + s = s->parent; + } + return s->nestingLevel; +} + /* * EnterParallelMode */ diff --git a/src/include/access/xact.h b/src/include/access/xact.h index b2bc10ee041..7f11b919799 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -458,6 +458,7 @@ extern TimestampTz GetCurrentTransactionStopTimestamp(void); extern void SetCurrentStatementStartTimestamp(void); extern int GetCurrentTransactionNestLevel(void); extern bool TransactionIdIsCurrentTransactionId(TransactionId xid); +extern int GetTopReadOnlyTransactionNestLevel(void); extern void CommandCounterIncrement(void); extern void ForceSyncCommit(void); extern void StartTransactionCommand(void); -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2025-06-01 08:44 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2025-06-01 08:44 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Sun, May 25, 2025 at 2:39 PM Etsuro Fujita <[email protected]> wrote: > Here is a new version of the patch where I added a comment for a new > function, fixed indentation, and added the commit message. If there > are no objections, I will push this as a master-only fix, as noted in > the commit message. Pushed after extending the comment a little bit. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2026-02-25 10:22 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2026-02-25 10:22 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Sun, Jun 1, 2025 at 5:44 PM Etsuro Fujita <[email protected]> wrote: > On Sun, May 25, 2025 at 2:39 PM Etsuro Fujita <[email protected]> wrote: > > Here is a new version of the patch where I added a comment for a new > > function, fixed indentation, and added the commit message. If there > > are no objections, I will push this as a master-only fix, as noted in > > the commit message. > > Pushed after extending the comment a little bit. This was reverted in commit 7d4667c62. I'd like to re-propose it for v19, as mentioned in [1]. Attached is a new patch, in which I added to the documentation a note about login triggers executed on the remote side, as discussed in [1]. Other than that, no changes. I've added this to the upcoming CF. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/CAPmGK14yYoO2iBg7SoT3WdnAKoCpWy_LPdoTDepJ21-Yf8TiKA%40mail.gma... Attachments: [application/octet-stream] v6-0001-postgres_fdw-Inherit-the-local-transaction-s-access-.patch (16.3K, 2-v6-0001-postgres_fdw-Inherit-the-local-transaction-s-access-.patch) download | inline diff: diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index add673a4776..3ce8e596a9e 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -59,6 +59,7 @@ typedef struct ConnCacheEntry /* Remaining fields are invalid when conn is NULL: */ int xact_depth; /* 0 = no xact open, 1 = main xact open, 2 = * one level of subxact open, etc */ + bool xact_read_only; /* xact r/o state */ bool have_prep_stmt; /* have we prepared any stmts in this xact? */ bool have_error; /* have any subxacts aborted in this xact? */ bool changing_xact_state; /* xact state change in process */ @@ -85,6 +86,12 @@ static unsigned int prep_stmt_number = 0; /* tracks whether any work is needed in callback functions */ static bool xact_got_connection = false; +/* + * tracks the nesting level of the topmost read-only transaction determined + * by GetTopReadOnlyTransactionNestLevel() + */ +static int top_read_only_level = 0; + /* custom wait event values, retrieved from shared memory */ static uint32 pgfdw_we_cleanup_result = 0; static uint32 pgfdw_we_connect = 0; @@ -376,6 +383,7 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) /* Reset all transient state fields, to be sure all are clean */ entry->xact_depth = 0; + entry->xact_read_only = false; entry->have_prep_stmt = false; entry->have_error = false; entry->changing_xact_state = false; @@ -850,29 +858,81 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input) * those scans. A disadvantage is that we can't provide sane emulation of * READ COMMITTED behavior --- it would be nice if we had some other way to * control which remote queries share a snapshot. + * + * Note also that we always start the remote transaction with the same + * read/write and deferrable properties as the local transaction, and start + * the remote subtransaction with the same read/write property as the local + * subtransaction. */ static void begin_remote_xact(ConnCacheEntry *entry) { int curlevel = GetCurrentTransactionNestLevel(); - /* Start main transaction if we haven't yet */ + /* + * Set the nesting level of the topmost read-only transaction if the + * current transaction is read-only and we haven't yet. Once it's set, + * it's retained until that transaction is committed/aborted, and then + * reset (see pgfdw_xact_callback and pgfdw_subxact_callback). + */ + if (XactReadOnly) + { + if (top_read_only_level == 0) + top_read_only_level = GetTopReadOnlyTransactionNestLevel(); + Assert(top_read_only_level > 0); + } + else + Assert(top_read_only_level == 0); + + /* + * Start main transaction if we haven't yet; otherwise, change the + * already-started remote transaction/subtransaction to read-only if the + * local transaction/subtransaction have been done so after starting them + * and we haven't yet. + */ if (entry->xact_depth <= 0) { - const char *sql; + StringInfoData sql; + bool ro = (top_read_only_level == 1); elog(DEBUG3, "starting remote transaction on connection %p", entry->conn); + initStringInfo(&sql); + appendStringInfoString(&sql, "START TRANSACTION ISOLATION LEVEL "); if (IsolationIsSerializable()) - sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE"; + appendStringInfoString(&sql, "SERIALIZABLE"); else - sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ"; + appendStringInfoString(&sql, "REPEATABLE READ"); + if (ro) + appendStringInfoString(&sql, " READ ONLY"); + if (XactDeferrable) + appendStringInfoString(&sql, " DEFERRABLE"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth = 1; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } + else if (!entry->xact_read_only) + { + Assert(top_read_only_level == 0 || + entry->xact_depth <= top_read_only_level); + if (entry->xact_depth == top_read_only_level) + { + entry->changing_xact_state = true; + do_sql_command(entry->conn, "SET transaction_read_only = on"); + entry->xact_read_only = true; + entry->changing_xact_state = false; + } + } + else + Assert(top_read_only_level > 0 && + entry->xact_depth >= top_read_only_level); /* * If we're in a subtransaction, stack up savepoints to match our level. @@ -881,12 +941,21 @@ begin_remote_xact(ConnCacheEntry *entry) */ while (entry->xact_depth < curlevel) { - char sql[64]; + StringInfoData sql; + bool ro = (entry->xact_depth + 1 == top_read_only_level); - snprintf(sql, sizeof(sql), "SAVEPOINT s%d", entry->xact_depth + 1); + initStringInfo(&sql); + appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1); + if (ro) + appendStringInfoString(&sql, "; SET transaction_read_only = on"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth++; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } } @@ -1191,6 +1260,9 @@ pgfdw_xact_callback(XactEvent event, void *arg) /* Also reset cursor numbering for next transaction */ cursor_number = 0; + + /* Likewise for top_read_only_level */ + top_read_only_level = 0; } /* @@ -1289,6 +1361,10 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid, false); } } + + /* If in the topmost read-only transaction, reset top_read_only_level */ + if (curlevel == top_read_only_level) + top_read_only_level = 0; } /* @@ -1391,6 +1467,9 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) /* Reset state to show we're out of a transaction */ entry->xact_depth = 0; + /* Reset xact r/o state */ + entry->xact_read_only = false; + /* * If the connection isn't in a good idle state, it is marked as * invalid or keep_connections option of its server is disabled, then @@ -1411,6 +1490,10 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) { /* Reset state to show we're out of a subtransaction */ entry->xact_depth--; + + /* If in the topmost read-only transaction, reset xact r/o state */ + if (entry->xact_depth + 1 == top_read_only_level) + entry->xact_read_only = false; } } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 2ccb72c539a..73ba14960f3 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -12524,6 +12524,140 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; -- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +CREATE FOREIGN TABLE remt2 (f1 int, f2 text) + SERVER loopback2 OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +-- Clean up +DROP FOREIGN TABLE remt; +DROP FOREIGN TABLE remt2; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; +-- =================================================================== -- test parallel commit and parallel abort -- =================================================================== ALTER SERVER loopback OPTIONS (ADD parallel_commit 'true'); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 72d2d9c311b..4cefd834f6c 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -4287,6 +4287,84 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; +-- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +CREATE FOREIGN TABLE remt2 (f1 int, f2 text) + SERVER loopback2 OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); + +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ROLLBACK; + +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +-- Clean up +DROP FOREIGN TABLE remt; +DROP FOREIGN TABLE remt2; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; + -- =================================================================== -- test parallel commit and parallel abort -- =================================================================== diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index fcf10e4317e..f745b3d4ce0 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -1077,6 +1077,23 @@ postgres=# SELECT postgres_fdw_disconnect_all(); <productname>PostgreSQL</productname> release might modify these rules. </para> + <para> + The remote transaction is opened in the same read/write mode as the local + transaction: if the local transaction is <literal>READ ONLY</literal>, + the remote transaction is opened in <literal>READ ONLY</literal> mode, + otherwise it is opened in <literal>READ WRITE</literal> mode. + (This rule is also applied to remote and local subtransactions.) + Note that this does not prevent login triggers executed on the remote + server from writing. + </para> + + <para> + The remote transaction is also opened in the same deferrable mode as the + local transaction: if the local transaction is <literal>DEFERRABLE</literal>, + the remote transaction is opened in <literal>DEFERRABLE</literal> mode, + otherwise it is opened in <literal>NOT DEFERRABLE</literal> mode. + </para> + <para> Note that it is currently not supported by <filename>postgres_fdw</filename> to prepare the remote transaction for diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index eba4f063168..efc81fd93b6 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1045,6 +1045,34 @@ TransactionStartedDuringRecovery(void) return CurrentTransactionState->startedInRecovery; } +/* + * GetTopReadOnlyTransactionNestLevel + * + * Note: this will return zero when not inside any transaction or when neither + * a top-level transaction nor subtransactions are read-only, one when the + * top-level transaction is read-only, two when one level of subtransaction is + * read-only, etc. + * + * Note: subtransactions of the topmost read-only transaction are also + * read-only, because they inherit read-only mode from the transaction, and + * thus can't change to read-write mode. See check_transaction_read_only(). + */ +int +GetTopReadOnlyTransactionNestLevel(void) +{ + TransactionState s = CurrentTransactionState; + + if (!XactReadOnly) + return 0; + while (s->nestingLevel > 1) + { + if (!s->prevXactReadOnly) + return s->nestingLevel; + s = s->parent; + } + return s->nestingLevel; +} + /* * EnterParallelMode */ diff --git a/src/include/access/xact.h b/src/include/access/xact.h index f0b4d795071..a8cbdf247c8 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -459,6 +459,7 @@ extern TimestampTz GetCurrentTransactionStopTimestamp(void); extern void SetCurrentStatementStartTimestamp(void); extern int GetCurrentTransactionNestLevel(void); extern bool TransactionIdIsCurrentTransactionId(TransactionId xid); +extern int GetTopReadOnlyTransactionNestLevel(void); extern void CommandCounterIncrement(void); extern void ForceSyncCommit(void); extern void StartTransactionCommand(void); ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2026-03-04 23:52 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2026-03-04 23:52 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Feb 25, 2026 at 7:22 PM Etsuro Fujita <[email protected]> wrote: > This was reverted in commit 7d4667c62. I'd like to re-propose it for > v19, as mentioned in [1]. Attached is a new patch, in which I added > to the documentation a note about login triggers executed on the > remote side, as discussed in [1]. Other than that, no changes. I've > added this to the upcoming CF. Here is an updated version of the patch. Changes are: * On second thought, I think the name of the variable top_read_only_level added to connection.c by the patch is a bit long, so I renamed it to top_read_only. Does that make sense? Other than that, no code changes. * I also added/modified some comments. Comments welcome! Best regards, Etsuro Fujita Attachments: [application/octet-stream] v7-0001-postgres_fdw-Inherit-the-local-transaction-s-access-.patch (17.0K, 2-v7-0001-postgres_fdw-Inherit-the-local-transaction-s-access-.patch) download | inline diff: diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 311936406f2..62d50b6fafe 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -59,6 +59,7 @@ typedef struct ConnCacheEntry /* Remaining fields are invalid when conn is NULL: */ int xact_depth; /* 0 = no xact open, 1 = main xact open, 2 = * one level of subxact open, etc */ + bool xact_read_only; /* xact r/o state */ bool have_prep_stmt; /* have we prepared any stmts in this xact? */ bool have_error; /* have any subxacts aborted in this xact? */ bool changing_xact_state; /* xact state change in process */ @@ -85,6 +86,12 @@ static unsigned int prep_stmt_number = 0; /* tracks whether any work is needed in callback functions */ static bool xact_got_connection = false; +/* + * tracks the topmost read-only local transaction's nesting level determined + * by GetTopReadOnlyTransactionNestLevel() + */ +static int read_only_level = 0; + /* custom wait event values, retrieved from shared memory */ static uint32 pgfdw_we_cleanup_result = 0; static uint32 pgfdw_we_connect = 0; @@ -376,6 +383,7 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) /* Reset all transient state fields, to be sure all are clean */ entry->xact_depth = 0; + entry->xact_read_only = false; entry->have_prep_stmt = false; entry->have_error = false; entry->changing_xact_state = false; @@ -851,29 +859,105 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input) * those scans. A disadvantage is that we can't provide sane emulation of * READ COMMITTED behavior --- it would be nice if we had some other way to * control which remote queries share a snapshot. + * + * Note also that we always start the remote transaction with the same + * read/write and deferrable properties as the local transaction, and start + * the remote subtransaction with the same read/write property as the local + * subtransaction. */ static void begin_remote_xact(ConnCacheEntry *entry) { int curlevel = GetCurrentTransactionNestLevel(); - /* Start main transaction if we haven't yet */ + /* + * If the current local (sub)transaction is read-only, set the topmost + * read-only local transaction's nesting level if we haven't yet. + * + * Note: once it's set, it's retained until the topmost read-only local + * transaction is committed/aborted (see pgfdw_xact_callback and + * pgfdw_subxact_callback). + */ + if (XactReadOnly) + { + if (read_only_level == 0) + read_only_level = GetTopReadOnlyTransactionNestLevel(); + Assert(read_only_level > 0); + } + else + Assert(read_only_level == 0); + + /* + * Start main transaction if we haven't yet; otherwise, change the + * current remote (sub)transaction's read/write mode if needed. + */ if (entry->xact_depth <= 0) { - const char *sql; + /* + * This is the case when we haven't yet started a main transaction. + */ + StringInfoData sql; + bool ro = (read_only_level == 1); elog(DEBUG3, "starting remote transaction on connection %p", entry->conn); + initStringInfo(&sql); + appendStringInfoString(&sql, "START TRANSACTION ISOLATION LEVEL "); if (IsolationIsSerializable()) - sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE"; + appendStringInfoString(&sql, "SERIALIZABLE"); else - sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ"; + appendStringInfoString(&sql, "REPEATABLE READ"); + if (ro) + appendStringInfoString(&sql, " READ ONLY"); + if (XactDeferrable) + appendStringInfoString(&sql, " DEFERRABLE"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth = 1; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } + else if (!entry->xact_read_only) + { + /* + * The remote (sub)transaction has been opened in read-write mode. + */ + Assert(read_only_level == 0 || + entry->xact_depth <= read_only_level); + + /* + * If its depth is equal to read_only_level, it means that the + * corresponding local read-write (sub)transaction has changed to + * (topmost) read-only after starting it in that mode; change its mode + * to read-only in that case. + */ + if (entry->xact_depth == read_only_level) + { + entry->changing_xact_state = true; + do_sql_command(entry->conn, "SET transaction_read_only = on"); + entry->xact_read_only = true; + entry->changing_xact_state = false; + } + } + else + { + /* + * The remote (sub)transaction has been opened in read-only mode. + */ + Assert(read_only_level > 0 && + entry->xact_depth >= read_only_level); + + /* + * Since the corresponding local read-only (sub)transaction can't + * change to read-write anymore (see check_transaction_read_only), + * there is no need to do anything. + */ + } /* * If we're in a subtransaction, stack up savepoints to match our level. @@ -882,12 +966,21 @@ begin_remote_xact(ConnCacheEntry *entry) */ while (entry->xact_depth < curlevel) { - char sql[64]; + StringInfoData sql; + bool ro = (entry->xact_depth + 1 == read_only_level); - snprintf(sql, sizeof(sql), "SAVEPOINT s%d", entry->xact_depth + 1); + initStringInfo(&sql); + appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1); + if (ro) + appendStringInfoString(&sql, "; SET transaction_read_only = on"); entry->changing_xact_state = true; - do_sql_command(entry->conn, sql); + do_sql_command(entry->conn, sql.data); entry->xact_depth++; + if (ro) + { + Assert(!entry->xact_read_only); + entry->xact_read_only = true; + } entry->changing_xact_state = false; } } @@ -1192,6 +1285,9 @@ pgfdw_xact_callback(XactEvent event, void *arg) /* Also reset cursor numbering for next transaction */ cursor_number = 0; + + /* Likewise for read_only_level */ + read_only_level = 0; } /* @@ -1290,6 +1386,10 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid, false); } } + + /* If in read_only_level, reset it */ + if (curlevel == read_only_level) + read_only_level = 0; } /* @@ -1392,6 +1492,9 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) /* Reset state to show we're out of a transaction */ entry->xact_depth = 0; + /* Reset xact r/o state */ + entry->xact_read_only = false; + /* * If the connection isn't in a good idle state, it is marked as * invalid or keep_connections option of its server is disabled, then @@ -1412,6 +1515,10 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel) { /* Reset state to show we're out of a subtransaction */ entry->xact_depth--; + + /* If in read_only_level, reset xact r/o state */ + if (entry->xact_depth + 1 == read_only_level) + entry->xact_read_only = false; } } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 2ccb72c539a..8e17dfe029a 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -12524,6 +12524,142 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; -- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +CREATE FOREIGN TABLE remt2 (f1 int, f2 text) + SERVER loopback2 OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +-- Exercise abort code paths in pgfdw_xact_callback/pgfdw_subxact_callback +-- in situations where multiple connections are involved +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work + f1 | f2 +----+-------- + 1 | foofoo + 2 | barbar +(2 rows) + +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ERROR: cannot execute UPDATE in a read-only transaction +CONTEXT: SQL function "locf" statement 1 +remote SQL command: SELECT f1, f2 FROM public.locv +ROLLBACK; +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; + f1 | f2 +----+----- + 1 | foo + 2 | bar +(2 rows) + +COMMIT; +-- Clean up +DROP FOREIGN TABLE remt; +DROP FOREIGN TABLE remt2; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; +-- =================================================================== -- test parallel commit and parallel abort -- =================================================================== ALTER SERVER loopback OPTIONS (ADD parallel_commit 'true'); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 72d2d9c311b..4e9794b347f 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -4287,6 +4287,86 @@ SELECT count(*) FROM remote_application_name DROP FOREIGN TABLE remote_application_name; DROP VIEW my_application_name; +-- =================================================================== +-- test read-only and/or deferrable transactions +-- =================================================================== +CREATE TABLE loct (f1 int, f2 text); +CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS + 'UPDATE public.loct SET f2 = f2 || f2 RETURNING *'; +CREATE VIEW locv AS SELECT t.* FROM locf() t; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'locv'); +CREATE FOREIGN TABLE remt2 (f1 int, f2 text) + SERVER loopback2 OPTIONS (table_name 'locv'); +INSERT INTO loct VALUES (1, 'foo'), (2, 'bar'); + +START TRANSACTION READ ONLY; +SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt; -- should fail +ROLLBACK; + +-- Exercise abort code paths in pgfdw_xact_callback/pgfdw_subxact_callback +-- in situations where multiple connections are involved +START TRANSACTION; +SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ROLLBACK TO s; +RELEASE SAVEPOINT s; +SELECT * FROM remt; -- should work +SET transaction_read_only = on; +SELECT * FROM remt2; -- should fail +ROLLBACK; + +DROP FOREIGN TABLE remt; +CREATE FOREIGN TABLE remt (f1 int, f2 text) + SERVER loopback OPTIONS (table_name 'loct'); + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY DEFERRABLE; +SELECT * FROM remt; +COMMIT; + +-- Clean up +DROP FOREIGN TABLE remt; +DROP FOREIGN TABLE remt2; +DROP VIEW locv; +DROP FUNCTION locf(); +DROP TABLE loct; + -- =================================================================== -- test parallel commit and parallel abort -- =================================================================== diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index fcf10e4317e..f745b3d4ce0 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -1077,6 +1077,23 @@ postgres=# SELECT postgres_fdw_disconnect_all(); <productname>PostgreSQL</productname> release might modify these rules. </para> + <para> + The remote transaction is opened in the same read/write mode as the local + transaction: if the local transaction is <literal>READ ONLY</literal>, + the remote transaction is opened in <literal>READ ONLY</literal> mode, + otherwise it is opened in <literal>READ WRITE</literal> mode. + (This rule is also applied to remote and local subtransactions.) + Note that this does not prevent login triggers executed on the remote + server from writing. + </para> + + <para> + The remote transaction is also opened in the same deferrable mode as the + local transaction: if the local transaction is <literal>DEFERRABLE</literal>, + the remote transaction is opened in <literal>DEFERRABLE</literal> mode, + otherwise it is opened in <literal>NOT DEFERRABLE</literal> mode. + </para> + <para> Note that it is currently not supported by <filename>postgres_fdw</filename> to prepare the remote transaction for diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index eba4f063168..39bf112b3fd 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1045,6 +1045,34 @@ TransactionStartedDuringRecovery(void) return CurrentTransactionState->startedInRecovery; } +/* + * GetTopReadOnlyTransactionNestLevel + * + * Note: this will return zero when not inside any transaction or when neither + * a top-level transaction nor subtransactions are read-only, one when the + * top-level transaction is read-only, two when one level of subtransaction is + * read-only, etc. + * + * Note: subtransactions of the topmost read-only transaction are also + * read-only, because they inherit read-only mode from the transaction, and + * thus can't change to read-write mode (see check_transaction_read_only). + */ +int +GetTopReadOnlyTransactionNestLevel(void) +{ + TransactionState s = CurrentTransactionState; + + if (!XactReadOnly) + return 0; + while (s->nestingLevel > 1) + { + if (!s->prevXactReadOnly) + return s->nestingLevel; + s = s->parent; + } + return s->nestingLevel; +} + /* * EnterParallelMode */ diff --git a/src/include/access/xact.h b/src/include/access/xact.h index f0b4d795071..a8cbdf247c8 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -459,6 +459,7 @@ extern TimestampTz GetCurrentTransactionStopTimestamp(void); extern void SetCurrentStatementStartTimestamp(void); extern int GetCurrentTransactionNestLevel(void); extern bool TransactionIdIsCurrentTransactionId(TransactionId xid); +extern int GetTopReadOnlyTransactionNestLevel(void); extern void CommandCounterIncrement(void); extern void ForceSyncCommit(void); extern void StartTransactionCommand(void); ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2026-03-05 03:10 Fujii Masao <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Fujii Masao @ 2026-03-05 03:10 UTC (permalink / raw) To: Etsuro Fujita <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 5, 2026 at 8:52 AM Etsuro Fujita <[email protected]> wrote: > > On Wed, Feb 25, 2026 at 7:22 PM Etsuro Fujita <[email protected]> wrote: > > This was reverted in commit 7d4667c62. I'd like to re-propose it for > > v19, as mentioned in [1]. Attached is a new patch, in which I added > > to the documentation a note about login triggers executed on the > > remote side, as discussed in [1]. Other than that, no changes. I've > > added this to the upcoming CF. > > Here is an updated version of the patch. Changes are: > > * On second thought, I think the name of the variable > top_read_only_level added to connection.c by the patch is a bit long, > so I renamed it to top_read_only. Does that make sense? Other than > that, no code changes. > * I also added/modified some comments. > > Comments welcome! I haven't yet realized the benefit from this change since I haven't encountered issues caused by the current behavior (i.e., a remote transaction starting in read-write mode while the corresponding local transaction on the standby is read-only). On the other hand, this change would force any remote transaction initiated by a standby transaction to start in read-only mode, completely preventing it from modifying data. Because transactions on a standby always start as read-only, the remote transaction would also always be read-only under this proposal, with no way to make it read-write. I'm concerned that this could break certain use cases without providing a clear benefit. Regards, -- Fujii Masao ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2026-03-07 18:37 Etsuro Fujita <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2026-03-07 18:37 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 5, 2026 at 12:11 PM Fujii Masao <[email protected]> wrote: > I haven't yet realized the benefit from this change since I haven't > encountered issues caused by the current behavior (i.e., a remote transaction > starting in read-write mode while the corresponding local transaction on > the standby is read-only). > > On the other hand, this change would force any remote transaction initiated by > a standby transaction to start in read-only mode, completely preventing it from > modifying data. Because transactions on a standby always start as read-only, > the remote transaction would also always be read-only under this proposal, > with no way to make it read-write. > > I'm concerned that this could break certain use cases without providing > a clear benefit. Thanks for the comments! The benefit is to make read-only transactions using postgres_fdw ensure read-only access. We discussed this in a Postgres developer meetup held at Yokohama in Japan last Friday. Let me explain again. Here is an example I used in that meetup to show the current behavior of such transactions: create server loopback foreign data wrapper postgres_fdw options (dbname 'postgres'); create user mapping for current_user server loopback; create table loct (f1 int, f2 text); create foreign table ft (f1 int, f2 text) server loopback options (table_name 'loct'); insert into ft values (1, 'foo'); insert into ft values (2, 'bar'); They disallow INSERT/UPDATE/DELETE, which is good: start transaction read only; insert into ft values (3, 'baz'); ERROR: cannot execute INSERT in a read-only transaction start transaction read only; update ft set f2 = 'xyzzy'; ERROR: cannot execute UPDATE in a read-only transaction start transaction read only; delete from ft; ERROR: cannot execute DELETE in a read-only transaction But if referencing foreign tables mapped to a remote view executing functions that modify data at the remote side, they can modify the data, which would be surprising: create function locf() returns setof loct language sql as 'update public.loct set f2 = f2 || f2 returning *'; create view locv as select t.* from locf() t; create foreign table fv (f1 int, f2 text) server loopback options (table_name 'locv'); start transaction read only; select * from fv; f1 | f2 ----+-------- 1 | foofoo 2 | barbar (2 rows) The root cause of this is that postgres_fdw opens a remote transaction in read-write mode even if the local transaction is read-only, so the patch I proposed addresses this by inheriting the read-only property from the local transaction. I didn't think of the use cases where postgres_fdw is used on a standby server, so I overlooked the breakage you mentioned above, but I got a lot of positive feedback from many participants regarding ensuring read-only access by that change. So I strongly believe the patch is the right way to go. I think it's unfortunate that it causes the breakage, though. I might be missing something, but I think a solution for such a use case is to use other DB integration tool like dblink. Anyway, I would like to know what other people think. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2026-03-09 03:47 Ashutosh Bapat <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Ashutosh Bapat @ 2026-03-09 03:47 UTC (permalink / raw) To: Etsuro Fujita <[email protected]>; +Cc: Fujii Masao <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Sun, Mar 8, 2026 at 12:07 AM Etsuro Fujita <[email protected]> wrote: > > On Thu, Mar 5, 2026 at 12:11 PM Fujii Masao <[email protected]> wrote: > > I haven't yet realized the benefit from this change since I haven't > > encountered issues caused by the current behavior (i.e., a remote transaction > > starting in read-write mode while the corresponding local transaction on > > the standby is read-only). > > > > On the other hand, this change would force any remote transaction initiated by > > a standby transaction to start in read-only mode, completely preventing it from > > modifying data. Because transactions on a standby always start as read-only, > > the remote transaction would also always be read-only under this proposal, > > with no way to make it read-write. > > > > I'm concerned that this could break certain use cases without providing > > a clear benefit. > > Thanks for the comments! > > The benefit is to make read-only transactions using postgres_fdw > ensure read-only access. We discussed this in a Postgres developer > meetup held at Yokohama in Japan last Friday. Let me explain again. > Here is an example I used in that meetup to show the current behavior > of such transactions: > > create server loopback > foreign data wrapper postgres_fdw > options (dbname 'postgres'); > create user mapping for current_user > server loopback; > create table loct (f1 int, f2 text); > create foreign table ft (f1 int, f2 text) > server loopback > options (table_name 'loct'); > insert into ft values (1, 'foo'); > insert into ft values (2, 'bar'); > > They disallow INSERT/UPDATE/DELETE, which is good: > > start transaction read only; > insert into ft values (3, 'baz'); > ERROR: cannot execute INSERT in a read-only transaction > > start transaction read only; > update ft set f2 = 'xyzzy'; > ERROR: cannot execute UPDATE in a read-only transaction > > start transaction read only; > delete from ft; > ERROR: cannot execute DELETE in a read-only transaction > > But if referencing foreign tables mapped to a remote view executing > functions that modify data at the remote side, they can modify the > data, which would be surprising: > > create function locf() returns setof loct language sql as > 'update public.loct set f2 = f2 || f2 returning *'; > create view locv as select t.* from locf() t; > create foreign table fv (f1 int, f2 text) > server loopback > options (table_name 'locv'); > > start transaction read only; > select * from fv; > f1 | f2 > ----+-------- > 1 | foofoo > 2 | barbar > (2 rows) > > The root cause of this is that postgres_fdw opens a remote transaction > in read-write mode even if the local transaction is read-only, so the > patch I proposed addresses this by inheriting the read-only property > from the local transaction. > > I didn't think of the use cases where postgres_fdw is used on a > standby server, so I overlooked the breakage you mentioned above, but > I got a lot of positive feedback from many participants regarding > ensuring read-only access by that change. So I strongly believe the > patch is the right way to go. I think it's unfortunate that it causes > the breakage, though. I might be missing something, but I think a > solution for such a use case is to use other DB integration tool like > dblink. If the primary doesn't allow modifying data in the foreign table in a read-only transaction, a standby shouldn't do that either. The users who are expecting a read-only transaction to protect against any writes to the foreign data on primary will also expect so on the standby. If users want to use standby's ability to modify foreign data for the sake of load balancing, that's a reasonable ask. However, we need to figure out whether it's common enough to support. That information is not readily available. I doubt that it's a common usecase. If this fix breaks such applications, we will come to know its spread. And such applications can use dblink. Alternately we can add the option which I and Tom didn't like [1]. But I feel we should do that only if there are complaints. It's going to be painful to those users who experience application breakage. To ease that pain we should highlight this as a compatibility break change in the beta release notes, giving users a chance to complain during beta cycle so that we can fix it by GA. If others know that the current behaviour has a widespread consumption, and they can provide backing data, adding the option right away is better. [1] postgr.es/m/CAExHW5vOH-=1KhaL8S4xVVzSozvrbmbBVg97p0obwEW3sD57Cw@mail.gmail.com -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2026-03-09 11:41 Etsuro Fujita <[email protected]> parent: Ashutosh Bapat <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2026-03-09 11:41 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Fujii Masao <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 9, 2026 at 12:47 PM Ashutosh Bapat <[email protected]> wrote: > If the primary doesn't allow modifying data in the foreign table in a > read-only transaction, a standby shouldn't do that either. The users > who are expecting a read-only transaction to protect against any > writes to the foreign data on primary will also expect so on the > standby. If users want to use standby's ability to modify foreign data > for the sake of load balancing, that's a reasonable ask. However, we > need to figure out whether it's common enough to support. That > information is not readily available. I doubt that it's a common > usecase. If this fix breaks such applications, we will come to know > its spread. And such applications can use dblink. Alternately we can > add the option which I and Tom didn't like [1]. But I feel we should > do that only if there are complaints. It's going to be painful to > those users who experience application breakage. To ease that pain we > should highlight this as a compatibility break change in the beta > release notes, giving users a chance to complain during beta cycle so > that we can fix it by GA. > > If others know that the current behaviour has a widespread > consumption, and they can provide backing data, adding the option > right away is better. +1; I agree with you 100%. Thanks for the comments! Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2026-03-27 11:41 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Etsuro Fujita @ 2026-03-27 11:41 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Fujii Masao <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 9, 2026 at 8:41 PM Etsuro Fujita <[email protected]> wrote: > On Mon, Mar 9, 2026 at 12:47 PM Ashutosh Bapat > <[email protected]> wrote: > > If the primary doesn't allow modifying data in the foreign table in a > > read-only transaction, a standby shouldn't do that either. The users > > who are expecting a read-only transaction to protect against any > > writes to the foreign data on primary will also expect so on the > > standby. If users want to use standby's ability to modify foreign data > > for the sake of load balancing, that's a reasonable ask. However, we > > need to figure out whether it's common enough to support. That > > information is not readily available. I doubt that it's a common > > usecase. If this fix breaks such applications, we will come to know > > its spread. And such applications can use dblink. Alternately we can > > add the option which I and Tom didn't like [1]. But I feel we should > > do that only if there are complaints. It's going to be painful to > > those users who experience application breakage. To ease that pain we > > should highlight this as a compatibility break change in the beta > > release notes, giving users a chance to complain during beta cycle so > > that we can fix it by GA. > > > > If others know that the current behaviour has a widespread > > consumption, and they can provide backing data, adding the option > > right away is better. > > +1; I agree with you 100%. Barring objections, I'll commit the patch early next week. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw @ 2026-04-05 10:02 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Etsuro Fujita @ 2026-04-05 10:02 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Fujii Masao <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Mar 27, 2026 at 8:41 PM Etsuro Fujita <[email protected]> wrote: > Barring objections, I'll commit the patch early next week. Pushed. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2026-04-05 10:02 UTC | newest] Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-03-02 11:44 Options to control remote transactions’ access/deferrable modes in postgres_fdw Etsuro Fujita <[email protected]> 2025-03-02 12:01 ` Etsuro Fujita <[email protected]> 2025-03-03 12:51 ` Ashutosh Bapat <[email protected]> 2025-03-03 15:49 ` Tom Lane <[email protected]> 2025-03-25 10:31 ` Etsuro Fujita <[email protected]> 2025-03-27 12:25 ` Ashutosh Bapat <[email protected]> 2025-03-30 10:14 ` Etsuro Fujita <[email protected]> 2025-05-04 12:30 ` Etsuro Fujita <[email protected]> 2025-05-08 08:50 ` Etsuro Fujita <[email protected]> 2025-05-25 05:39 ` Etsuro Fujita <[email protected]> 2025-06-01 08:44 ` Etsuro Fujita <[email protected]> 2026-02-25 10:22 ` Etsuro Fujita <[email protected]> 2026-03-04 23:52 ` Etsuro Fujita <[email protected]> 2026-03-05 03:10 ` Fujii Masao <[email protected]> 2026-03-07 18:37 ` Etsuro Fujita <[email protected]> 2026-03-09 03:47 ` Ashutosh Bapat <[email protected]> 2026-03-09 11:41 ` Etsuro Fujita <[email protected]> 2026-03-27 11:41 ` Etsuro Fujita <[email protected]> 2026-04-05 10:02 ` Etsuro Fujita <[email protected]> 2025-03-25 10:44 ` Etsuro Fujita <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox