public inbox for [email protected]
help / color / mirror / Atom feedFrom: Etsuro Fujita <[email protected]>
To: Tom Lane <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw
Date: Tue, 25 Mar 2025 11:31:08 +0100
Message-ID: <CAPmGK14Btk0odkH6vwBhBGjCexmmWcM_D3DG0pJtObj8k_Unag@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAPmGK16n_hcUUWuOdmeUS+w4Q6dZvTEDHb=OP=5JBzo-M3QmpQ@mail.gmail.com>
<CAExHW5vOH-=1KhaL8S4xVVzSozvrbmbBVg97p0obwEW3sD57Cw@mail.gmail.com>
<[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);
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: <CAPmGK14Btk0odkH6vwBhBGjCexmmWcM_D3DG0pJtObj8k_Unag@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