public inbox for [email protected]
help / color / mirror / Atom feedFrom: Etsuro Fujita <[email protected]>
To: Ashutosh Bapat <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw
Date: Sun, 4 May 2025 21:30:42 +0900
Message-ID: <CAPmGK14Zn9NwKPPmBrUyN9ALBJa2nv75-LOVKNBgRN=ehYnKkw@mail.gmail.com> (raw)
In-Reply-To: <CAPmGK16pn+k0tLNR8NvZdtiADpnjZPYK-HuaxwOmzHzUgTE7_A@mail.gmail.com>
References: <CAPmGK16n_hcUUWuOdmeUS+w4Q6dZvTEDHb=OP=5JBzo-M3QmpQ@mail.gmail.com>
<CAExHW5vOH-=1KhaL8S4xVVzSozvrbmbBVg97p0obwEW3sD57Cw@mail.gmail.com>
<[email protected]>
<CAPmGK14Btk0odkH6vwBhBGjCexmmWcM_D3DG0pJtObj8k_Unag@mail.gmail.com>
<CAExHW5vVwZdvW1Dfy3oJXVDBffJUvAwhaCw3HCSC38B1ie_H1A@mail.gmail.com>
<CAPmGK16pn+k0tLNR8NvZdtiADpnjZPYK-HuaxwOmzHzUgTE7_A@mail.gmail.com>
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);
view thread (20+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw
In-Reply-To: <CAPmGK14Zn9NwKPPmBrUyN9ALBJa2nv75-LOVKNBgRN=ehYnKkw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox