agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedpostgres_fdw: transaction mode inheritance corner cases
17+ messages / 5 participants
[nested] [flat]
* postgres_fdw: transaction mode inheritance corner cases
@ 2026-09-10 00:33 Fujii Masao <masao.fujii@gmail.com>
0 siblings, 2 replies; 17+ messages in thread
From: Fujii Masao @ 2026-09-10 00:33 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
Commit de28140ded8 introduced transaction mode inheritance in
postgres_fdw, but I found several potential issues with it.
We should add this as PostgreSQL 19 Open Item?
(1) READ ONLY does not take effect for an existing cursor
DECLARE initializes the foreign scan and opens the remote transaction.
FETCH reuses that connection without calling begin_remote_xact(), so
a subsequent mode change is not propagated to the remote server.
In the following example, the query executed by FETCH runs in READ WRITE
mode on the remote server even though the local transaction is READ ONLY:
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft;
SET TRANSACTION READ ONLY;
FETCH ALL FROM c;
COMMIT;
(2) READ WRITE and NOT DEFERRABLE are not always inherited
begin_remote_xact() adds READ ONLY and DEFERRABLE when applicable,
but does not explicitly specify READ WRITE or NOT DEFERRABLE. Those
modes therefore depend on the defaults on the remote server.
This seems to conflict with the postgres_fdw docs:
The remote transaction is opened in the same read/write mode as the
local transaction: if the local transaction is READ ONLY, the remote
transaction is opened in READ ONLY mode, otherwise it is opened in READ
WRITE mode.
(3) DEFERRABLE breaks queries against PostgreSQL 9.0 and older
When the remote server is PostgreSQL 9.0 or older, the remote
START TRANSACTION can fail with a syntax error at DEFERRABLE, since
that option is not supported there. Since the docs still says that
read-only access is supported back to PostgreSQL 8.1, this case
should be handled.
(4) A loopback query can wait indefinitely after switching to READ ONLY
BEGIN ISOLATION LEVEL SERIALIZABLE READ WRITE DEFERRABLE;
SELECT * FROM t;
SET TRANSACTION READ ONLY;
SELECT * FROM ft;
ROLLBACK;
In this example, the foreign SELECT waits indefinitely.
The local transaction remains READ WRITE in SSI after taking its first
snapshot. The remote READ ONLY DEFERRABLE transaction waits for the
local transaction to finish before obtaining a safe snapshot, while the
local transaction waits for the remote query.
I'm not sure whether this is something we should fix or just consider an
operational mistake, but I wanted to share the case.
(5) Deferred remote triggers can write after switching to READ ONLY
pgfdw_xact_callback() sends COMMIT without synchronizing the
read-only mode, so a deferred trigger on the remote server can still run
in READ WRITE mode.
BEGIN;
INSERT INTO ft VALUES (...);
SET TRANSACTION READ ONLY;
COMMIT;
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-09-10 09:24 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Fujii Masao <masao.fujii@gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: Etsuro Fujita @ 2026-09-10 09:24 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Sep 10, 2026 at 9:34 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> Commit de28140ded8 introduced transaction mode inheritance in
> postgres_fdw, but I found several potential issues with it.
Thanks for the report! Will look into the issues.
> We should add this as PostgreSQL 19 Open Item?
I added this to the open item list.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-09-16 15:44 Nathan Bossart <nathandbossart@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Nathan Bossart @ 2026-09-16 15:44 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
[RMT hat]
On Thu, Sep 10, 2026 at 06:24:55PM +0900, Etsuro Fujita wrote:
> On Thu, Sep 10, 2026 at 9:34 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>> Commit de28140ded8 introduced transaction mode inheritance in
>> postgres_fdw, but I found several potential issues with it.
>
> Thanks for the report! Will look into the issues.
>
>> We should add this as PostgreSQL 19 Open Item?
>
> I added this to the open item list.
Will these issues be fixed before code freeze for 19beta4 on Saturday?
Does commit de28140ded8 need to be reverted?
--
nathan
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-09-16 20:08 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Etsuro Fujita @ 2026-09-16 20:08 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Sep 17, 2026 at 12:44 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> On Thu, Sep 10, 2026 at 06:24:55PM +0900, Etsuro Fujita wrote:
> > On Thu, Sep 10, 2026 at 9:34 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> >> Commit de28140ded8 introduced transaction mode inheritance in
> >> postgres_fdw, but I found several potential issues with it.
> >
> > Thanks for the report! Will look into the issues.
> >
> >> We should add this as PostgreSQL 19 Open Item?
> >
> > I added this to the open item list.
>
> Will these issues be fixed before code freeze for 19beta4 on Saturday?
> Does commit de28140ded8 need to be reverted?
Unfortunately, I don’t think I will be able to fix these by then. I'm
planning to propose (the first version of) a fix next week as soon as
possible.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-09-30 10:45 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Fujii Masao <masao.fujii@gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: Etsuro Fujita @ 2026-09-30 10:45 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Sep 10, 2026 at 9:34 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> (1) READ ONLY does not take effect for an existing cursor
>
> DECLARE initializes the foreign scan and opens the remote transaction.
> FETCH reuses that connection without calling begin_remote_xact(), so
> a subsequent mode change is not propagated to the remote server.
Fixed.
> (2) READ WRITE and NOT DEFERRABLE are not always inherited
>
> begin_remote_xact() adds READ ONLY and DEFERRABLE when applicable,
> but does not explicitly specify READ WRITE or NOT DEFERRABLE. Those
> modes therefore depend on the defaults on the remote server.
Fixed.
> (3) DEFERRABLE breaks queries against PostgreSQL 9.0 and older
>
> When the remote server is PostgreSQL 9.0 or older, the remote
> START TRANSACTION can fail with a syntax error at DEFERRABLE, since
> that option is not supported there. Since the docs still says that
> read-only access is supported back to PostgreSQL 8.1, this case
> should be handled.
Fixed.
> (4) A loopback query can wait indefinitely after switching to READ ONLY
>
> BEGIN ISOLATION LEVEL SERIALIZABLE READ WRITE DEFERRABLE;
> SELECT * FROM t;
> SET TRANSACTION READ ONLY;
> SELECT * FROM ft;
> ROLLBACK;
>
> In this example, the foreign SELECT waits indefinitely.
>
> The local transaction remains READ WRITE in SSI after taking its first
> snapshot. The remote READ ONLY DEFERRABLE transaction waits for the
> local transaction to finish before obtaining a safe snapshot, while the
> local transaction waits for the remote query.
>
> I'm not sure whether this is something we should fix or just consider an
> operational mistake, but I wanted to share the case.
This is expected behavior, so I would say it would be that mistake.
> (5) Deferred remote triggers can write after switching to READ ONLY
>
> pgfdw_xact_callback() sends COMMIT without synchronizing the
> read-only mode, so a deferred trigger on the remote server can still run
> in READ WRITE mode.
Fixed.
Attached is a patch for that. I will add test cases for these in the
next version.
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] postgres_fdw-Fix-corner-cases-in-xact-mode-inh-v1.patch (10.7K, ../../CAPmGK15biMpA0Qpjpt_3iTP++Wh=789jYF-bRnwpH-RO=RdiFg@mail.gmail.com/2-postgres_fdw-Fix-corner-cases-in-xact-mode-inh-v1.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index b5d4cf3dccc..00c264d0276 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -112,6 +112,20 @@ static uint32 pgfdw_we_get_result = 0;
*/
#define RETRY_CANCEL_TIMEOUT 1000
+/*
+ * Macro for constructing commit command to be sent
+ *
+ * We synchronize the read/write mode before committing remote transactions
+ * so deferred triggers on remote servers can run in the right mode.
+ */
+#define CONSTRUCT_COMMIT_COMMAND(sql, entry) \
+ do { \
+ if ((read_only_level > 0) && !(entry)->xact_read_only) \
+ strcpy((sql), "SET TRANSACTION READ ONLY; COMMIT TRANSACTION"); \
+ else \
+ strcpy((sql), "COMMIT TRANSACTION"); \
+ } while(0)
+
/* Macro for constructing abort command to be sent */
#define CONSTRUCT_ABORT_COMMAND(sql, entry, toplevel) \
do { \
@@ -397,7 +411,8 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user)
entry->mapping_hashvalue =
GetSysCacheHashValue1(USERMAPPINGOID,
ObjectIdGetDatum(user->umid));
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
+ entry->state.entry = entry;
/*
* Determine whether to keep the connection that we're about to make here
@@ -929,6 +944,7 @@ begin_remote_xact(ConnCacheEntry *entry)
*/
StringInfoData sql;
bool ro = (read_only_level == 1);
+ int remoteversion = PQserverVersion(entry->conn);
elog(DEBUG3, "starting remote transaction on connection %p",
entry->conn);
@@ -941,8 +957,15 @@ begin_remote_xact(ConnCacheEntry *entry)
appendStringInfoString(&sql, "REPEATABLE READ");
if (ro)
appendStringInfoString(&sql, " READ ONLY");
- if (XactDeferrable)
- appendStringInfoString(&sql, " DEFERRABLE");
+ else
+ appendStringInfoString(&sql, " READ WRITE");
+ if (remoteversion >= 90100)
+ {
+ if (XactDeferrable)
+ appendStringInfoString(&sql, " DEFERRABLE");
+ else
+ appendStringInfoString(&sql, " NOT DEFERRABLE");
+ }
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
entry->xact_depth = 1;
@@ -971,7 +994,7 @@ begin_remote_xact(ConnCacheEntry *entry)
if (entry->xact_depth == read_only_level)
{
entry->changing_xact_state = true;
- do_sql_command(entry->conn, "SET transaction_read_only = on");
+ do_sql_command(entry->conn, "SET TRANSACTION READ ONLY");
entry->xact_read_only = true;
entry->changing_xact_state = false;
}
@@ -1004,7 +1027,7 @@ begin_remote_xact(ConnCacheEntry *entry)
initStringInfo(&sql);
appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1);
if (ro)
- appendStringInfoString(&sql, "; SET transaction_read_only = on");
+ appendStringInfoString(&sql, "; SET TRANSACTION READ ONLY");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
entry->xact_depth++;
@@ -1061,6 +1084,20 @@ GetPrepStmtNumber(PGconn *conn)
return ++prep_stmt_number;
}
+/*
+ * Exported version of begin_remote_xact().
+ *
+ * This can be called for connections on which begin_remote_xact() has started
+ * a remote transaction.
+ */
+void
+pgfdw_begin_remote_xact(ConnCacheEntry *entry)
+{
+ Assert(entry);
+ Assert(entry->xact_depth > 0);
+ begin_remote_xact(entry);
+}
+
/*
* Submit a query and wait for the result.
*
@@ -1077,6 +1114,14 @@ pgfdw_exec_query(PGconn *conn, const char *query, PgFdwConnState *state)
if (state && state->pendingAreq)
process_pending_request(state->pendingAreq);
+ /*
+ * Ensure the local and remote (sub)transactions are synchronized. Note
+ * that we need to do this because this function can be called from open
+ * cursors, bypassing begin_remote_xact().
+ */
+ if (state)
+ pgfdw_begin_remote_xact(state->entry);
+
if (!PQsendQuery(conn, query))
return NULL;
return pgfdw_get_result(conn);
@@ -1184,6 +1229,19 @@ pgfdw_xact_callback(XactEvent event, void *arg)
if (!xact_got_connection)
return;
+ /*
+ * The local transaction may have become read-only since the last remote
+ * operation, so ensure read_only_level is set for later processing.
+ */
+ if (XactReadOnly)
+ {
+ if (read_only_level == 0)
+ read_only_level = 1;
+ Assert(read_only_level == 1);
+ }
+ else
+ Assert(read_only_level == 0);
+
/*
* Scan all connection cache entries to find open remote transactions, and
* close them.
@@ -1200,6 +1258,8 @@ pgfdw_xact_callback(XactEvent event, void *arg)
/* If it has an open remote transaction, try to close it */
if (entry->xact_depth > 0)
{
+ char sql[100];
+
elog(DEBUG3, "closing remote transaction on connection %p",
entry->conn);
@@ -1215,14 +1275,17 @@ pgfdw_xact_callback(XactEvent event, void *arg)
pgfdw_reject_incomplete_xact_state_change(entry);
/* Commit all remote transactions during pre-commit */
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
entry->changing_xact_state = true;
if (entry->parallel_commit)
{
- do_sql_command_begin(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command_begin(entry->conn, sql);
pending_entries = lappend(pending_entries, entry);
continue;
}
- do_sql_command(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command(entry->conn, sql);
+ if ((read_only_level > 0) && !entry->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/*
@@ -1929,10 +1992,10 @@ pgfdw_abort_cleanup(ConnCacheEntry *entry, bool toplevel)
* If pendingAreq of the per-connection state is not NULL, it means that
* an asynchronous fetch begun by fetch_more_data_begin() was not done
* successfully and thus the per-connection state was not reset in
- * fetch_more_data(); in that case reset the per-connection state here.
+ * fetch_more_data(); in that case reset pendingAreq here.
*/
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* Disarm changing_xact_state if it all worked */
entry->changing_xact_state = false;
@@ -2019,6 +2082,8 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
*/
foreach(lc, pending_entries)
{
+ char sql[100];
+
entry = (ConnCacheEntry *) lfirst(lc);
Assert(entry->changing_xact_state);
@@ -2027,7 +2092,10 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
* We might already have received the result on the socket, so pass
* consume_input=true to try to consume it first
*/
- do_sql_command_end(entry->conn, "COMMIT TRANSACTION", true);
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
+ do_sql_command_end(entry->conn, sql, true);
+ if ((read_only_level > 0) && !(entry)->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/* Do a DEALLOCATE ALL in parallel if needed */
@@ -2221,9 +2289,9 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
entry->have_error = false;
}
- /* Reset the per-connection state if needed */
+ /* Reset pendingAreq here if any */
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* We're done with this entry; unset the changing_xact_state flag */
entry->changing_xact_state = false;
@@ -2266,9 +2334,9 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
entry->have_prep_stmt = false;
entry->have_error = false;
- /* Reset the per-connection state if needed */
+ /* Reset pendingAreq here if any */
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* We're done with this entry; unset the changing_xact_state flag */
entry->changing_xact_state = false;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2bcff4b26b4..445eb3d4d79 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4058,6 +4058,13 @@ create_cursor(ForeignScanState *node)
if (fsstate->conn_state->pendingAreq)
process_pending_request(fsstate->conn_state->pendingAreq);
+ /*
+ * Ensure the local and remote (sub)transactions are synchronized. Note
+ * that we need to do this because this function can be called from open
+ * cursors, bypassing begin_remote_xact().
+ */
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
+
/*
* Construct array of query parameter values in text format. We do the
* conversions in the short-lived per-tuple context, so as not to cause a
@@ -4147,7 +4154,7 @@ fetch_more_data(ForeignScanState *node)
if (PQresultStatus(res) != PGRES_TUPLES_OK)
pgfdw_report_error(res, conn, fsstate->query);
- /* Reset per-connection state */
+ /* Reset the pending asynchronous request */
fsstate->conn_state->pendingAreq = NULL;
}
else
@@ -8840,6 +8847,13 @@ fetch_more_data_begin(AsyncRequest *areq)
Assert(!fsstate->conn_state->pendingAreq);
+ /*
+ * Ensure the local and remote (sub)transactions are synchronized. Note
+ * that we need to do this because this function can be called from open
+ * cursors, bypassing begin_remote_xact().
+ */
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
+
/* Create the cursor synchronously. */
if (!fsstate->cursor_exists)
create_cursor(node);
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index da7da1c2ea9..b9b460141f9 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -146,7 +146,8 @@ typedef struct PgFdwRelationInfo
*/
typedef struct PgFdwConnState
{
- AsyncRequest *pendingAreq; /* pending async request */
+ AsyncRequest *pendingAreq; /* pending async request */
+ struct ConnCacheEntry *entry; /* link to containing ConnCacheEntry */
} PgFdwConnState;
/*
@@ -173,6 +174,7 @@ extern void ReleaseConnection(PGconn *conn);
extern unsigned int GetCursorNumber(PGconn *conn);
extern unsigned int GetPrepStmtNumber(PGconn *conn);
extern void do_sql_command(PGconn *conn, const char *sql);
+extern void pgfdw_begin_remote_xact(struct ConnCacheEntry *entry);
extern PGresult *pgfdw_get_result(PGconn *conn);
extern PGresult *pgfdw_exec_query(PGconn *conn, const char *query,
PgFdwConnState *state);
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index fe4e6478e28..24fd5e342b4 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -1162,6 +1162,7 @@ CREATE SUBSCRIPTION my_subscription SERVER subscription_server PUBLICATION testp
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.
+ (This rule is only applied to remote servers 9.1 and newer.)
</para>
<para>
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-09-30 18:27 Matheus Alcantara <matheusssilv97@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 2 replies; 17+ messages in thread
From: Matheus Alcantara @ 2026-09-30 18:27 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed Sep 30, 2026 at 7:45 AM -03, Etsuro Fujita wrote:
> Attached is a patch for that. I will add test cases for these in the
> next version.
>
Hi, thanks for the patch! I tested it and I think that I may have found
two issues:
1: read-write local transactions can no longer query a hot standby
If I create a foreign server pointing to a standby, sending an explicit
READ WRITE makes the standby reject the remote START TRANSACTION. A
plain SELECT from a foreign table on a standby now fails, including in
autocommit, because the local transaction is read-write by default. It
works on unpatched master.
Repro:
-- on the primary (port 5433 is running a standby server)
create extension postgres_fdw;
create table t(a int);
insert into t values (1),(2);
create server sb foreign data wrapper postgres_fdw
options (dbname 'postgres', port '5433');
create user mapping for current_user server sb;
create foreign table fsb(a int) server sb options (table_name 't');
select * from fsb;
ERROR: 0A000: cannot set transaction read-write mode during recovery
CONTEXT: remote SQL command: START TRANSACTION ISOLATION LEVEL REPEATABLE READ READ WRITE NOT DEFERRABLE
begin read only;
select * from fsb; -- works
commit;
I'm not sure how much common is querying a standby through postgres_fdw,
but I've already seen some cases, so I'm wondering if this needs some
handling, what do you think?
2: a foreign cursor first fetched in a rolled-back savepoint breaks at COMMIT
Repro:
begin; declare c cursor for select * from ft;
savepoint s1; fetch 1 from c; rollback to s1;
fetch all from c; commit;
ERROR: 34000: cursor "c1" does not exist
CONTEXT: remote SQL command: CLOSE c1
This also works on unpatched master. There, the remote cursor is created
lazily at the first FETCH, without a remote savepoint, so it lives at
remote level 1 and survives ROLLBACK TO s1. With the patch, the new
begin_remote_xact() call in create_cursor opens a remote SAVEPOINT s2
and creates the cursor inside it. ROLLBACK TO s1 then destroys the
remote cursor while the local side still thinks it exists.
If the first FETCH happens before the savepoint, the same script works
with the patch. I think the remote cursor needs to be created at the
level where the scan started, or the remote/local cursor state needs to
be reconciled some other way.
Also, I didn't tested this case but on execute_foreign_modify and
direct-modify results are read without going through
begin_remote_xact(). So I'm wondering if a volatile function that runs
SET TRANSACTION READ ONLY in the middle of a single INSERT ... SELECT
f() could bypass the sync.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-01 16:47 Nikolay Samokhvalov <nik@postgres.ai>
parent: Matheus Alcantara <matheusssilv97@gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: Nikolay Samokhvalov @ 2026-10-01 16:47 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: Etsuro Fujita <etsuro.fujita@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Sep 30, 2026 at 11:27 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> With the patch, the new begin_remote_xact() call in create_cursor opens a
> remote SAVEPOINT s2 and creates the cursor inside it. ROLLBACK TO s1 then
> destroys the remote cursor while the local side still thinks it exists.
Thanks, Matheus. My AI harness for testing reproduced this on
REL_19_STABLE at 9e73b209 with Etsuro's v1 patch and prepared the
attached incremental patch. It declares the remote cursor before
advancing the remote savepoint level, then synchronizes the transaction
mode before FETCH. The second FETCH fails with 34000 on v1 and succeeds
with this patch.
The postgres_fdw suite passes (4/4). This does not address the hot
standby case, which needs a separate fix.
Nik
Attachments:
[application/x-patch] v1-0001-postgres_fdw-preserve-outer-cursors-on-rollback.patch (4.0K, ../../CAM527d_iZYxz9osL1Wra5=n0+WPTv3N3X-_3_XgVsd+EkTscdQ@mail.gmail.com/2-v1-0001-postgres_fdw-preserve-outer-cursors-on-rollback.patch)
download | inline diff:
From e0e670e4e95d04a8a37e03c86c363bc75e56ff6b Mon Sep 17 00:00:00 2001
From: Nikolay Samokhvalov <nik@postgres.ai>
Date: Wed, 30 Sep 2026 16:36:41 -0700
Subject: [PATCH] postgres_fdw: Preserve outer cursors across subtransaction
rollback
---
.../postgres_fdw/expected/postgres_fdw.out | 20 +++++++++++++++
contrib/postgres_fdw/postgres_fdw.c | 25 ++++++++-----------
contrib/postgres_fdw/sql/postgres_fdw.sql | 11 ++++++++
3 files changed, 41 insertions(+), 15 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 3a17e02..953b01c 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -13375,3 +13375,23 @@ RESET client_min_messages;
DROP FUNCTION wait_for_backend_termination(int);
DROP FOREIGN TABLE remote_backend_pid;
DROP VIEW my_backend_pid;
+-- A cursor opened before a savepoint can first be fetched within it.
+ALTER FOREIGN TABLE ft1 OPTIONS (ADD fetch_size '1');
+BEGIN;
+DECLARE c CURSOR FOR SELECT c1 FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+FETCH c;
+ c1
+----
+ 1
+(1 row)
+
+ROLLBACK TO SAVEPOINT s;
+FETCH c;
+ c1
+----
+ 3
+(1 row)
+
+COMMIT;
+ALTER FOREIGN TABLE ft1 OPTIONS (DROP fetch_size);
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4e0f096..209a422 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3826,13 +3826,6 @@ create_cursor(ForeignScanState *node)
if (fsstate->conn_state->pendingAreq)
process_pending_request(fsstate->conn_state->pendingAreq);
- /*
- * Ensure the local and remote (sub)transactions are synchronized. Note
- * that we need to do this because this function can be called from open
- * cursors, bypassing begin_remote_xact().
- */
- pgfdw_begin_remote_xact(fsstate->conn_state->entry);
-
/*
* Construct array of query parameter values in text format. We do the
* conversions in the short-lived per-tuple context, so as not to cause a
@@ -3852,7 +3845,10 @@ create_cursor(ForeignScanState *node)
MemoryContextSwitchTo(oldcontext);
}
- /* Construct the DECLARE CURSOR command */
+ /*
+ * Declare the cursor before advancing the remote savepoint level, so a
+ * cursor opened by an outer local transaction survives rollback here.
+ */
initStringInfo(&buf);
appendStringInfo(&buf, "DECLARE c%u CURSOR FOR\n%s",
fsstate->cursor_number, fsstate->query);
@@ -8324,17 +8320,16 @@ fetch_more_data_begin(AsyncRequest *areq)
Assert(!fsstate->conn_state->pendingAreq);
- /*
- * Ensure the local and remote (sub)transactions are synchronized. Note
- * that we need to do this because this function can be called from open
- * cursors, bypassing begin_remote_xact().
- */
- pgfdw_begin_remote_xact(fsstate->conn_state->entry);
-
/* Create the cursor synchronously. */
if (!fsstate->cursor_exists)
create_cursor(node);
+ /*
+ * Synchronize the remote (sub)transaction after declaring the cursor, so
+ * an outer-level local cursor survives rollback of this subtransaction.
+ */
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
+
/* We will send this query, but not wait for the response. */
snprintf(sql, sizeof(sql), "FETCH %d FROM c%u",
fsstate->fetch_size, fsstate->cursor_number);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 4b19c50..4cbb97b 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -4814,3 +4814,14 @@ RESET client_min_messages;
DROP FUNCTION wait_for_backend_termination(int);
DROP FOREIGN TABLE remote_backend_pid;
DROP VIEW my_backend_pid;
+
+-- A cursor opened before a savepoint can first be fetched within it.
+ALTER FOREIGN TABLE ft1 OPTIONS (ADD fetch_size '1');
+BEGIN;
+DECLARE c CURSOR FOR SELECT c1 FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+FETCH c;
+ROLLBACK TO SAVEPOINT s;
+FETCH c;
+COMMIT;
+ALTER FOREIGN TABLE ft1 OPTIONS (DROP fetch_size);
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-01 17:30 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Matheus Alcantara <matheusssilv97@gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: Etsuro Fujita @ 2026-10-01 17:30 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Oct 1, 2026 at 3:27 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> 1: read-write local transactions can no longer query a hot standby
Ah, I think this is too restrictive, so I changed my mind; I'd like to
propose to keep the current behavior and instead modify the
documentation like this:
> 2: a foreign cursor first fetched in a rolled-back savepoint breaks at COMMIT
>
> Repro:
>
> begin; declare c cursor for select * from ft;
> savepoint s1; fetch 1 from c; rollback to s1;
> fetch all from c; commit;
> ERROR: 34000: cursor "c1" does not exist
> CONTEXT: remote SQL command: CLOSE c1
>
> This also works on unpatched master. There, the remote cursor is created
> lazily at the first FETCH, without a remote savepoint, so it lives at
> remote level 1 and survives ROLLBACK TO s1. With the patch, the new
> begin_remote_xact() call in create_cursor opens a remote SAVEPOINT s2
> and creates the cursor inside it. ROLLBACK TO s1 then destroys the
> remote cursor while the local side still thinks it exists.
Reproduced here. I think your analysis is correct, but I noticed that
this is an existing issue in postgres_fdw even in v18 and older. Here
is an example:
begin;
declare c cursor for select * from ft1;
savepoint s;
select * from ft1;
a | b
---+---
1 | 1
2 | 2
(2 rows)
fetch 2 from c;
a | b
---+---
1 | 1
2 | 2
(2 rows)
rollback to s;
release savepoint s;
commit;
ERROR: cursor "c1" does not exist
CONTEXT: remote SQL command: CLOSE c1
> Also, I didn't tested this case but on execute_foreign_modify and
> direct-modify results are read without going through
> begin_remote_xact(). So I'm wondering if a volatile function that runs
> SET TRANSACTION READ ONLY in the middle of a single INSERT ... SELECT
> f() could bypass the sync.
DML statements are prevented on the local server if in READ ONLY mode,
so those functions are never run in that mode. No?
Thanks for the review!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-01 17:32 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Nikolay Samokhvalov <nik@postgres.ai>
0 siblings, 1 reply; 17+ messages in thread
From: Etsuro Fujita @ 2026-10-01 17:32 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Matheus Alcantara <matheusssilv97@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Oct 2, 2026 at 1:47 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
> On Wed, Sep 30, 2026 at 11:27 AM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
> > With the patch, the new begin_remote_xact() call in create_cursor opens a
> > remote SAVEPOINT s2 and creates the cursor inside it. ROLLBACK TO s1 then
> > destroys the remote cursor while the local side still thinks it exists.
>
> Thanks, Matheus. My AI harness for testing reproduced this on
> REL_19_STABLE at 9e73b209 with Etsuro's v1 patch and prepared the
> attached incremental patch. It declares the remote cursor before
> advancing the remote savepoint level, then synchronizes the transaction
> mode before FETCH. The second FETCH fails with 34000 on v1 and succeeds
> with this patch.
Will look into the patch. Thanks!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-01 17:37 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Etsuro Fujita @ 2026-10-01 17:37 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Oct 2, 2026 at 2:30 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Thu, Oct 1, 2026 at 3:27 AM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
> > 1: read-write local transactions can no longer query a hot standby
>
> Ah, I think this is too restrictive, so I changed my mind; I'd like to
> propose to keep the current behavior and instead modify the
> documentation like this:
Sorry I forgot this example:
Local READ ONLY transactions propagate their read-only mode to remote
sessions. (This is also applied to subtransactions.) Note that this
does not prevent login triggers executed on remote servers from
writing.
Also, local DEFERRABLE transactions propagate their deferrable mode to
remote sessions. (This is only applied to remote servers 9.1 and
newer.)
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-02 15:54 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Etsuro Fujita @ 2026-10-02 15:54 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Matheus Alcantara <matheusssilv97@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Oct 2, 2026 at 2:32 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Fri, Oct 2, 2026 at 1:47 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
> > My AI harness for testing reproduced this on
> > REL_19_STABLE at 9e73b209 with Etsuro's v1 patch and prepared the
> > attached incremental patch. It declares the remote cursor before
> > advancing the remote savepoint level, then synchronizes the transaction
> > mode before FETCH. The second FETCH fails with 34000 on v1 and succeeds
> > with this patch.
>
> Will look into the patch.
I think the patch assumes that create_cursor() is called at the same
transaction nesting depth as the local cursor, but that doesn't always
hold; for eg, the case I showed yesterday, that doesn't hold, so it
still fails. So it's a partial solution as proposed. Rather than
complicating the code, I'd like to propose to fix this by just
disallowing first fetching of a cursor within a deeper subtransaction
than it was created in. Here is an updated version for that. This is
an existing issue, so I split it into two:
* v2-0001-Fix-open-cursor-handling.patch
This addresses the existing issue by disallowing the fetching (and the
issue #1 reported by Fujii-san as a side effect).
* v2-0002-Fix-xact-prop-issues.patch
This addresses the remaining issues #2, #3 and #5 reported by
Fujii-san (#4 is not a bug). I will add test cases next.
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] v2-0001-Fix-open-cursor-handling.patch (9.8K, ../../CAPmGK14sOuOXdMCk6FeZtogGWuf4vF65H1PtdsskCZ8ERprPrw@mail.gmail.com/2-v2-0001-Fix-open-cursor-handling.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index b5d4cf3dccc..652fa4a943d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -397,7 +397,8 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user)
entry->mapping_hashvalue =
GetSysCacheHashValue1(USERMAPPINGOID,
ObjectIdGetDatum(user->umid));
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
+ entry->state.entry = entry;
/*
* Determine whether to keep the connection that we're about to make here
@@ -1061,6 +1062,20 @@ GetPrepStmtNumber(PGconn *conn)
return ++prep_stmt_number;
}
+/*
+ * Exported version of begin_remote_xact().
+ *
+ * This can be called for connections on which begin_remote_xact() has started
+ * a remote transaction.
+ */
+void
+pgfdw_begin_remote_xact(ConnCacheEntry *entry)
+{
+ Assert(entry);
+ Assert(entry->xact_depth > 0);
+ begin_remote_xact(entry);
+}
+
/*
* Submit a query and wait for the result.
*
@@ -1077,6 +1092,13 @@ pgfdw_exec_query(PGconn *conn, const char *query, PgFdwConnState *state)
if (state && state->pendingAreq)
process_pending_request(state->pendingAreq);
+ /*
+ * Second, synchronize the local/remote transactions. Note that we need
+ * to do this because this function can be called from open cursors.
+ */
+ if (state)
+ pgfdw_begin_remote_xact(state->entry);
+
if (!PQsendQuery(conn, query))
return NULL;
return pgfdw_get_result(conn);
@@ -1929,10 +1951,10 @@ pgfdw_abort_cleanup(ConnCacheEntry *entry, bool toplevel)
* If pendingAreq of the per-connection state is not NULL, it means that
* an asynchronous fetch begun by fetch_more_data_begin() was not done
* successfully and thus the per-connection state was not reset in
- * fetch_more_data(); in that case reset the per-connection state here.
+ * fetch_more_data(); in that case reset pendingAreq here.
*/
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* Disarm changing_xact_state if it all worked */
entry->changing_xact_state = false;
@@ -2221,9 +2243,9 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
entry->have_error = false;
}
- /* Reset the per-connection state if needed */
+ /* Reset pendingAreq here if any */
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* We're done with this entry; unset the changing_xact_state flag */
entry->changing_xact_state = false;
@@ -2266,9 +2288,9 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
entry->have_prep_stmt = false;
entry->have_error = false;
- /* Reset the per-connection state if needed */
+ /* Reset pendingAreq here if any */
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* We're done with this entry; unset the changing_xact_state flag */
entry->changing_xact_state = false;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 739f43af7bb..3aab56b0642 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -5332,6 +5332,8 @@ ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
+-- + cursors opened before a savepoint are disallowed to be first
+-- fetched within it
-- ===================================================================
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
@@ -5371,6 +5373,12 @@ SELECT * FROM ft1 ORDER BY c1 LIMIT 1;
(1 row)
COMMIT;
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+FETCH c;
+ERROR: cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in
+ABORT;
-- ===================================================================
-- test handling of collations
-- ===================================================================
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2bcff4b26b4..b1d75487c67 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -17,6 +17,7 @@
#include "access/htup_details.h"
#include "access/sysattr.h"
#include "access/table.h"
+#include "access/xact.h"
#include "catalog/pg_opfamily.h"
#include "commands/defrem.h"
#include "commands/explain_format.h"
@@ -189,6 +190,7 @@ typedef struct PgFdwScanState
FmgrInfo *param_flinfo; /* output conversion functions for them */
List *param_exprs; /* executable expressions for param values */
const char **param_values; /* textual values of query parameters */
+ int created_at; /* xact depth at which the scan was created */
/* for storing result tuples */
HeapTuple *tuples; /* array of currently-retrieved tuples */
@@ -1763,6 +1765,9 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
fsstate->cursor_number = GetCursorNumber(fsstate->conn);
fsstate->cursor_exists = false;
+ /* Get the current local transaction's nesting depth */
+ fsstate->created_at = GetCurrentTransactionNestLevel();
+
/* Get private info created by planner functions. */
fsstate->query = strVal(list_nth(fsplan->fdw_private,
FdwScanPrivateSelectSql));
@@ -4054,10 +4059,21 @@ create_cursor(ForeignScanState *node)
StringInfoData buf;
PGresult *res;
+ if (fsstate->created_at < GetCurrentTransactionNestLevel())
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in")));
+
/* First, process a pending asynchronous request, if any. */
if (fsstate->conn_state->pendingAreq)
process_pending_request(fsstate->conn_state->pendingAreq);
+ /*
+ * Second, synchronize the local/remote transactions. Note that we need
+ * to do this because this function can be called from open cursors.
+ */
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
+
/*
* Construct array of query parameter values in text format. We do the
* conversions in the short-lived per-tuple context, so as not to cause a
@@ -4147,7 +4163,7 @@ fetch_more_data(ForeignScanState *node)
if (PQresultStatus(res) != PGRES_TUPLES_OK)
pgfdw_report_error(res, conn, fsstate->query);
- /* Reset per-connection state */
+ /* Reset the pending asynchronous request */
fsstate->conn_state->pendingAreq = NULL;
}
else
@@ -4394,6 +4410,9 @@ create_foreign_modify(EState *estate,
* result if any. (This is the shared guts of postgresExecForeignInsert,
* postgresExecForeignBatchInsert, postgresExecForeignUpdate, and
* postgresExecForeignDelete.)
+ *
+ * Note: this function is never called from open cursors, thus no need to
+ * synchronize the local/remote transactions.
*/
static TupleTableSlot **
execute_foreign_modify(EState *estate,
@@ -4832,6 +4851,9 @@ rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist)
/*
* Execute a direct UPDATE/DELETE statement.
+ *
+ * Note: this function is never called from open cursors, thus no need to
+ * synchronize the local/remote transactions.
*/
static void
execute_dml_stmt(ForeignScanState *node)
@@ -8840,9 +8862,14 @@ fetch_more_data_begin(AsyncRequest *areq)
Assert(!fsstate->conn_state->pendingAreq);
- /* Create the cursor synchronously. */
+ /*
+ * Create the cursor synchronously if not already done. Otherwise,
+ * synchronize the local/remote transactions before the data fetch.
+ */
if (!fsstate->cursor_exists)
create_cursor(node);
+ else
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
/* We will send this query, but not wait for the response. */
snprintf(sql, sizeof(sql), "FETCH %d FROM c%u",
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index da7da1c2ea9..b9b460141f9 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -146,7 +146,8 @@ typedef struct PgFdwRelationInfo
*/
typedef struct PgFdwConnState
{
- AsyncRequest *pendingAreq; /* pending async request */
+ AsyncRequest *pendingAreq; /* pending async request */
+ struct ConnCacheEntry *entry; /* link to containing ConnCacheEntry */
} PgFdwConnState;
/*
@@ -173,6 +174,7 @@ extern void ReleaseConnection(PGconn *conn);
extern unsigned int GetCursorNumber(PGconn *conn);
extern unsigned int GetPrepStmtNumber(PGconn *conn);
extern void do_sql_command(PGconn *conn, const char *sql);
+extern void pgfdw_begin_remote_xact(struct ConnCacheEntry *entry);
extern PGresult *pgfdw_get_result(PGconn *conn);
extern PGresult *pgfdw_exec_query(PGconn *conn, const char *query,
PgFdwConnState *state);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index f1ca3204382..9c271953206 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1635,6 +1635,8 @@ ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
+-- + cursors opened before a savepoint are disallowed to be first
+-- fetched within it
-- ===================================================================
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
@@ -1650,6 +1652,12 @@ FETCH c;
SELECT * FROM ft1 ORDER BY c1 LIMIT 1;
COMMIT;
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+FETCH c;
+ABORT;
+
-- ===================================================================
-- test handling of collations
-- ===================================================================
[application/octet-stream] v2-0002-Fix-xact-prop-issues.patch (5.9K, ../../CAPmGK14sOuOXdMCk6FeZtogGWuf4vF65H1PtdsskCZ8ERprPrw@mail.gmail.com/3-v2-0002-Fix-xact-prop-issues.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 652fa4a943d..77aec0fd9eb 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -112,6 +112,20 @@ static uint32 pgfdw_we_get_result = 0;
*/
#define RETRY_CANCEL_TIMEOUT 1000
+/*
+ * Macro for constructing commit command to be sent
+ *
+ * We synchronize the read/write mode before committing remote transactions
+ * so deferred triggers on remote servers can run in the right mode.
+ */
+#define CONSTRUCT_COMMIT_COMMAND(sql, entry) \
+ do { \
+ if ((read_only_level > 0) && !(entry)->xact_read_only) \
+ strcpy((sql), "SET TRANSACTION READ ONLY; COMMIT TRANSACTION"); \
+ else \
+ strcpy((sql), "COMMIT TRANSACTION"); \
+ } while(0)
+
/* Macro for constructing abort command to be sent */
#define CONSTRUCT_ABORT_COMMAND(sql, entry, toplevel) \
do { \
@@ -942,7 +956,7 @@ begin_remote_xact(ConnCacheEntry *entry)
appendStringInfoString(&sql, "REPEATABLE READ");
if (ro)
appendStringInfoString(&sql, " READ ONLY");
- if (XactDeferrable)
+ if (XactDeferrable && PQserverVersion(entry->conn) >= 90100)
appendStringInfoString(&sql, " DEFERRABLE");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
@@ -972,7 +986,7 @@ begin_remote_xact(ConnCacheEntry *entry)
if (entry->xact_depth == read_only_level)
{
entry->changing_xact_state = true;
- do_sql_command(entry->conn, "SET transaction_read_only = on");
+ do_sql_command(entry->conn, "SET TRANSACTION READ ONLY");
entry->xact_read_only = true;
entry->changing_xact_state = false;
}
@@ -1005,7 +1019,7 @@ begin_remote_xact(ConnCacheEntry *entry)
initStringInfo(&sql);
appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1);
if (ro)
- appendStringInfoString(&sql, "; SET transaction_read_only = on");
+ appendStringInfoString(&sql, "; SET TRANSACTION READ ONLY");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
entry->xact_depth++;
@@ -1206,6 +1220,24 @@ pgfdw_xact_callback(XactEvent event, void *arg)
if (!xact_got_connection)
return;
+ /*
+ * If we are called for pre-commit cleanup, ensure read_only_level is set
+ * for later processing. Note that we need to do this because the local
+ * transaction may have become read-only since the last remote operation.
+ */
+ if (event == XACT_EVENT_PARALLEL_PRE_COMMIT ||
+ event == XACT_EVENT_PRE_COMMIT)
+ {
+ if (XactReadOnly)
+ {
+ if (read_only_level == 0)
+ read_only_level = 1;
+ Assert(read_only_level == 1);
+ }
+ else
+ Assert(read_only_level == 0);
+ }
+
/*
* Scan all connection cache entries to find open remote transactions, and
* close them.
@@ -1222,6 +1254,8 @@ pgfdw_xact_callback(XactEvent event, void *arg)
/* If it has an open remote transaction, try to close it */
if (entry->xact_depth > 0)
{
+ char sql[100];
+
elog(DEBUG3, "closing remote transaction on connection %p",
entry->conn);
@@ -1237,14 +1271,17 @@ pgfdw_xact_callback(XactEvent event, void *arg)
pgfdw_reject_incomplete_xact_state_change(entry);
/* Commit all remote transactions during pre-commit */
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
entry->changing_xact_state = true;
if (entry->parallel_commit)
{
- do_sql_command_begin(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command_begin(entry->conn, sql);
pending_entries = lappend(pending_entries, entry);
continue;
}
- do_sql_command(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command(entry->conn, sql);
+ if ((read_only_level > 0) && !entry->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/*
@@ -2041,6 +2078,8 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
*/
foreach(lc, pending_entries)
{
+ char sql[100];
+
entry = (ConnCacheEntry *) lfirst(lc);
Assert(entry->changing_xact_state);
@@ -2049,7 +2088,10 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
* We might already have received the result on the socket, so pass
* consume_input=true to try to consume it first
*/
- do_sql_command_end(entry->conn, "COMMIT TRANSACTION", true);
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
+ do_sql_command_end(entry->conn, sql, true);
+ if ((read_only_level > 0) && !(entry)->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/* Do a DEALLOCATE ALL in parallel if needed */
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index fe4e6478e28..01577d8d69b 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -1148,20 +1148,17 @@ CREATE SUBSCRIPTION my_subscription SERVER subscription_server PUBLICATION testp
</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.)
+ Local <literal>READ ONLY</literal> transactions propagate their read-only
+ mode to remote sessions.
+ (This rule is also applied to 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.
+ Also, local <literal>DEFERRABLE</literal> transactions propagate their
+ deferrable mode to remote sessions.
+ (This rule is only applied to remote servers 9.1 and newer.)
</para>
<para>
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-03 09:38 Matheus Alcantara <matheusssilv97@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 2 replies; 17+ messages in thread
From: Matheus Alcantara @ 2026-10-03 09:38 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Fri Oct 2, 2026 at 12:54 PM -03, Etsuro Fujita wrote:
> On Fri, Oct 2, 2026 at 2:32 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> On Fri, Oct 2, 2026 at 1:47 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
>> > My AI harness for testing reproduced this on
>> > REL_19_STABLE at 9e73b209 with Etsuro's v1 patch and prepared the
>> > attached incremental patch. It declares the remote cursor before
>> > advancing the remote savepoint level, then synchronizes the transaction
>> > mode before FETCH. The second FETCH fails with 34000 on v1 and succeeds
>> > with this patch.
>>
>> Will look into the patch.
>
> I think the patch assumes that create_cursor() is called at the same
> transaction nesting depth as the local cursor, but that doesn't always
> hold; for eg, the case I showed yesterday, that doesn't hold, so it
> still fails. So it's a partial solution as proposed. Rather than
> complicating the code, I'd like to propose to fix this by just
> disallowing first fetching of a cursor within a deeper subtransaction
> than it was created in. Here is an updated version for that. This is
> an existing issue, so I split it into two:
>
> * v2-0001-Fix-open-cursor-handling.patch
> This addresses the existing issue by disallowing the fetching (and the
> issue #1 reported by Fujii-san as a side effect).
>
> * v2-0002-Fix-xact-prop-issues.patch
> This addresses the remaining issues #2, #3 and #5 reported by
> Fujii-san (#4 is not a bug). I will add test cases next.
>
Thanks for the v2 patches! I tested them and the hot standby issue is
fixed by 0002, and the deferred trigger case works as expected. I found
two problems with 0001, 0002 looks good to me.
1: the check in 0001 doesn't cover sibling savepoints
Since created_at only keeps the nesting depth, a cursor declared in one
savepoint and first fetched in a sibling savepoint at the same depth
passes the check:
begin;
savepoint s1;
declare c cursor for select * from ft;
release s1;
savepoint s2;
fetch 1 from c;
rollback to s2;
fetch all from c;
commit;
ERROR: 34000: cursor "c1" does not exist
CONTEXT: remote SQL command: CLOSE c1
2: it rejects cases that work on master
A PL/pgSQL refcursor that is opened outside an exception block and first
fetched inside it works on master, but fails with 0001 with the new
error. It also fails if the exception handler swallows errors: the new
error is hidden inside the block, the portal is left failed, and the
later fetch outside the block fails with a confusing 'portal "<unnamed
portal 2>" cannot be run'. On master this works because no remote
savepoint exists yet, so the remote cursor is created at remote level 1.
If we keep this restriction, I'm wondering if needs a documentation note
and a release note, what do you think?
I'm attaching a prototype 0003 on top of 0001 and 0002 that fixes both
issues that I've mention. The idea is that the real problem is not the
local nesting depth, but whether the remote savepoint depth at the time
of the DECLARE is deeper than the level where the local cursor lives,
since rolling back a remote savepoint at or below that depth destroys
the remote cursor. So 0003 declares the remote cursor before
synchronizing the remote savepoint level, and raises the error only if
the current remote depth is deeper than the level of the local cursor.
To know that level it records the subtransaction ID and the nesting
level when the scan is created; if that subtransaction was already
released, it conservatively assumes the cursor lives at the top level.
With this, the cases above work, and your earlier example, where another
scan advances the remote savepoint level before the first fetch, still
errors. The postgres_fdw tests pass, and I adjusted the cursor tests of
0001 accordingly.
It's a prototype, I haven't tested the async path beyond the existing
tests. What do you think?
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index b5d4cf3dccc..652fa4a943d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -397,7 +397,8 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user)
entry->mapping_hashvalue =
GetSysCacheHashValue1(USERMAPPINGOID,
ObjectIdGetDatum(user->umid));
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
+ entry->state.entry = entry;
/*
* Determine whether to keep the connection that we're about to make here
@@ -1061,6 +1062,20 @@ GetPrepStmtNumber(PGconn *conn)
return ++prep_stmt_number;
}
+/*
+ * Exported version of begin_remote_xact().
+ *
+ * This can be called for connections on which begin_remote_xact() has started
+ * a remote transaction.
+ */
+void
+pgfdw_begin_remote_xact(ConnCacheEntry *entry)
+{
+ Assert(entry);
+ Assert(entry->xact_depth > 0);
+ begin_remote_xact(entry);
+}
+
/*
* Submit a query and wait for the result.
*
@@ -1077,6 +1092,13 @@ pgfdw_exec_query(PGconn *conn, const char *query, PgFdwConnState *state)
if (state && state->pendingAreq)
process_pending_request(state->pendingAreq);
+ /*
+ * Second, synchronize the local/remote transactions. Note that we need
+ * to do this because this function can be called from open cursors.
+ */
+ if (state)
+ pgfdw_begin_remote_xact(state->entry);
+
if (!PQsendQuery(conn, query))
return NULL;
return pgfdw_get_result(conn);
@@ -1929,10 +1951,10 @@ pgfdw_abort_cleanup(ConnCacheEntry *entry, bool toplevel)
* If pendingAreq of the per-connection state is not NULL, it means that
* an asynchronous fetch begun by fetch_more_data_begin() was not done
* successfully and thus the per-connection state was not reset in
- * fetch_more_data(); in that case reset the per-connection state here.
+ * fetch_more_data(); in that case reset pendingAreq here.
*/
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* Disarm changing_xact_state if it all worked */
entry->changing_xact_state = false;
@@ -2221,9 +2243,9 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
entry->have_error = false;
}
- /* Reset the per-connection state if needed */
+ /* Reset pendingAreq here if any */
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* We're done with this entry; unset the changing_xact_state flag */
entry->changing_xact_state = false;
@@ -2266,9 +2288,9 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
entry->have_prep_stmt = false;
entry->have_error = false;
- /* Reset the per-connection state if needed */
+ /* Reset pendingAreq here if any */
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* We're done with this entry; unset the changing_xact_state flag */
entry->changing_xact_state = false;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 739f43af7bb..3aab56b0642 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -5332,6 +5332,8 @@ ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
+-- + cursors opened before a savepoint are disallowed to be first
+-- fetched within it
-- ===================================================================
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
@@ -5371,6 +5373,12 @@ SELECT * FROM ft1 ORDER BY c1 LIMIT 1;
(1 row)
COMMIT;
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+FETCH c;
+ERROR: cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in
+ABORT;
-- ===================================================================
-- test handling of collations
-- ===================================================================
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2bcff4b26b4..b1d75487c67 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -17,6 +17,7 @@
#include "access/htup_details.h"
#include "access/sysattr.h"
#include "access/table.h"
+#include "access/xact.h"
#include "catalog/pg_opfamily.h"
#include "commands/defrem.h"
#include "commands/explain_format.h"
@@ -189,6 +190,7 @@ typedef struct PgFdwScanState
FmgrInfo *param_flinfo; /* output conversion functions for them */
List *param_exprs; /* executable expressions for param values */
const char **param_values; /* textual values of query parameters */
+ int created_at; /* xact depth at which the scan was created */
/* for storing result tuples */
HeapTuple *tuples; /* array of currently-retrieved tuples */
@@ -1763,6 +1765,9 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
fsstate->cursor_number = GetCursorNumber(fsstate->conn);
fsstate->cursor_exists = false;
+ /* Get the current local transaction's nesting depth */
+ fsstate->created_at = GetCurrentTransactionNestLevel();
+
/* Get private info created by planner functions. */
fsstate->query = strVal(list_nth(fsplan->fdw_private,
FdwScanPrivateSelectSql));
@@ -4054,10 +4059,21 @@ create_cursor(ForeignScanState *node)
StringInfoData buf;
PGresult *res;
+ if (fsstate->created_at < GetCurrentTransactionNestLevel())
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in")));
+
/* First, process a pending asynchronous request, if any. */
if (fsstate->conn_state->pendingAreq)
process_pending_request(fsstate->conn_state->pendingAreq);
+ /*
+ * Second, synchronize the local/remote transactions. Note that we need
+ * to do this because this function can be called from open cursors.
+ */
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
+
/*
* Construct array of query parameter values in text format. We do the
* conversions in the short-lived per-tuple context, so as not to cause a
@@ -4147,7 +4163,7 @@ fetch_more_data(ForeignScanState *node)
if (PQresultStatus(res) != PGRES_TUPLES_OK)
pgfdw_report_error(res, conn, fsstate->query);
- /* Reset per-connection state */
+ /* Reset the pending asynchronous request */
fsstate->conn_state->pendingAreq = NULL;
}
else
@@ -4394,6 +4410,9 @@ create_foreign_modify(EState *estate,
* result if any. (This is the shared guts of postgresExecForeignInsert,
* postgresExecForeignBatchInsert, postgresExecForeignUpdate, and
* postgresExecForeignDelete.)
+ *
+ * Note: this function is never called from open cursors, thus no need to
+ * synchronize the local/remote transactions.
*/
static TupleTableSlot **
execute_foreign_modify(EState *estate,
@@ -4832,6 +4851,9 @@ rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist)
/*
* Execute a direct UPDATE/DELETE statement.
+ *
+ * Note: this function is never called from open cursors, thus no need to
+ * synchronize the local/remote transactions.
*/
static void
execute_dml_stmt(ForeignScanState *node)
@@ -8840,9 +8862,14 @@ fetch_more_data_begin(AsyncRequest *areq)
Assert(!fsstate->conn_state->pendingAreq);
- /* Create the cursor synchronously. */
+ /*
+ * Create the cursor synchronously if not already done. Otherwise,
+ * synchronize the local/remote transactions before the data fetch.
+ */
if (!fsstate->cursor_exists)
create_cursor(node);
+ else
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
/* We will send this query, but not wait for the response. */
snprintf(sql, sizeof(sql), "FETCH %d FROM c%u",
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index da7da1c2ea9..b9b460141f9 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -146,7 +146,8 @@ typedef struct PgFdwRelationInfo
*/
typedef struct PgFdwConnState
{
- AsyncRequest *pendingAreq; /* pending async request */
+ AsyncRequest *pendingAreq; /* pending async request */
+ struct ConnCacheEntry *entry; /* link to containing ConnCacheEntry */
} PgFdwConnState;
/*
@@ -173,6 +174,7 @@ extern void ReleaseConnection(PGconn *conn);
extern unsigned int GetCursorNumber(PGconn *conn);
extern unsigned int GetPrepStmtNumber(PGconn *conn);
extern void do_sql_command(PGconn *conn, const char *sql);
+extern void pgfdw_begin_remote_xact(struct ConnCacheEntry *entry);
extern PGresult *pgfdw_get_result(PGconn *conn);
extern PGresult *pgfdw_exec_query(PGconn *conn, const char *query,
PgFdwConnState *state);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index f1ca3204382..9c271953206 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1635,6 +1635,8 @@ ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
+-- + cursors opened before a savepoint are disallowed to be first
+-- fetched within it
-- ===================================================================
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
@@ -1650,6 +1652,12 @@ FETCH c;
SELECT * FROM ft1 ORDER BY c1 LIMIT 1;
COMMIT;
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+FETCH c;
+ABORT;
+
-- ===================================================================
-- test handling of collations
-- ===================================================================
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 652fa4a943d..77aec0fd9eb 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -112,6 +112,20 @@ static uint32 pgfdw_we_get_result = 0;
*/
#define RETRY_CANCEL_TIMEOUT 1000
+/*
+ * Macro for constructing commit command to be sent
+ *
+ * We synchronize the read/write mode before committing remote transactions
+ * so deferred triggers on remote servers can run in the right mode.
+ */
+#define CONSTRUCT_COMMIT_COMMAND(sql, entry) \
+ do { \
+ if ((read_only_level > 0) && !(entry)->xact_read_only) \
+ strcpy((sql), "SET TRANSACTION READ ONLY; COMMIT TRANSACTION"); \
+ else \
+ strcpy((sql), "COMMIT TRANSACTION"); \
+ } while(0)
+
/* Macro for constructing abort command to be sent */
#define CONSTRUCT_ABORT_COMMAND(sql, entry, toplevel) \
do { \
@@ -942,7 +956,7 @@ begin_remote_xact(ConnCacheEntry *entry)
appendStringInfoString(&sql, "REPEATABLE READ");
if (ro)
appendStringInfoString(&sql, " READ ONLY");
- if (XactDeferrable)
+ if (XactDeferrable && PQserverVersion(entry->conn) >= 90100)
appendStringInfoString(&sql, " DEFERRABLE");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
@@ -972,7 +986,7 @@ begin_remote_xact(ConnCacheEntry *entry)
if (entry->xact_depth == read_only_level)
{
entry->changing_xact_state = true;
- do_sql_command(entry->conn, "SET transaction_read_only = on");
+ do_sql_command(entry->conn, "SET TRANSACTION READ ONLY");
entry->xact_read_only = true;
entry->changing_xact_state = false;
}
@@ -1005,7 +1019,7 @@ begin_remote_xact(ConnCacheEntry *entry)
initStringInfo(&sql);
appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1);
if (ro)
- appendStringInfoString(&sql, "; SET transaction_read_only = on");
+ appendStringInfoString(&sql, "; SET TRANSACTION READ ONLY");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
entry->xact_depth++;
@@ -1206,6 +1220,24 @@ pgfdw_xact_callback(XactEvent event, void *arg)
if (!xact_got_connection)
return;
+ /*
+ * If we are called for pre-commit cleanup, ensure read_only_level is set
+ * for later processing. Note that we need to do this because the local
+ * transaction may have become read-only since the last remote operation.
+ */
+ if (event == XACT_EVENT_PARALLEL_PRE_COMMIT ||
+ event == XACT_EVENT_PRE_COMMIT)
+ {
+ if (XactReadOnly)
+ {
+ if (read_only_level == 0)
+ read_only_level = 1;
+ Assert(read_only_level == 1);
+ }
+ else
+ Assert(read_only_level == 0);
+ }
+
/*
* Scan all connection cache entries to find open remote transactions, and
* close them.
@@ -1222,6 +1254,8 @@ pgfdw_xact_callback(XactEvent event, void *arg)
/* If it has an open remote transaction, try to close it */
if (entry->xact_depth > 0)
{
+ char sql[100];
+
elog(DEBUG3, "closing remote transaction on connection %p",
entry->conn);
@@ -1237,14 +1271,17 @@ pgfdw_xact_callback(XactEvent event, void *arg)
pgfdw_reject_incomplete_xact_state_change(entry);
/* Commit all remote transactions during pre-commit */
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
entry->changing_xact_state = true;
if (entry->parallel_commit)
{
- do_sql_command_begin(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command_begin(entry->conn, sql);
pending_entries = lappend(pending_entries, entry);
continue;
}
- do_sql_command(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command(entry->conn, sql);
+ if ((read_only_level > 0) && !entry->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/*
@@ -2041,6 +2078,8 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
*/
foreach(lc, pending_entries)
{
+ char sql[100];
+
entry = (ConnCacheEntry *) lfirst(lc);
Assert(entry->changing_xact_state);
@@ -2049,7 +2088,10 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
* We might already have received the result on the socket, so pass
* consume_input=true to try to consume it first
*/
- do_sql_command_end(entry->conn, "COMMIT TRANSACTION", true);
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
+ do_sql_command_end(entry->conn, sql, true);
+ if ((read_only_level > 0) && !(entry)->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/* Do a DEALLOCATE ALL in parallel if needed */
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index fe4e6478e28..01577d8d69b 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -1148,20 +1148,17 @@ CREATE SUBSCRIPTION my_subscription SERVER subscription_server PUBLICATION testp
</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.)
+ Local <literal>READ ONLY</literal> transactions propagate their read-only
+ mode to remote sessions.
+ (This rule is also applied to 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.
+ Also, local <literal>DEFERRABLE</literal> transactions propagate their
+ deferrable mode to remote sessions.
+ (This rule is only applied to remote servers 9.1 and newer.)
</para>
<para>
diff -ru a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
--- a/contrib/postgres_fdw/connection.c 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/connection.c 2026-10-03 05:55:27
@@ -1091,6 +1091,16 @@
}
/*
+ * Return the nesting depth of the remote (sub)transaction currently open on
+ * the connection (0 if none).
+ */
+int
+pgfdw_remote_xact_depth(ConnCacheEntry *entry)
+{
+ return entry->xact_depth;
+}
+
+/*
* Submit a query and wait for the result.
*
* Since we don't use non-blocking mode, this can't process interrupts while
diff -ru a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
--- a/contrib/postgres_fdw/expected/postgres_fdw.out 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out 2026-10-03 05:55:27
@@ -5373,9 +5373,50 @@
(1 row)
COMMIT;
+-- first fetch within a savepoint is fine if remote savepoint level is
+-- not advanced beyond the one the cursor was created in
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
SAVEPOINT s;
+FETCH c;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
+(1 row)
+
+ROLLBACK TO s;
+FETCH c;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 2 | 2 | 00002 | Sat Jan 03 00:00:00 1970 PST | Sat Jan 03 00:00:00 1970 | 2 | 2 | foo
+(1 row)
+
+COMMIT;
+-- ... but not otherwise
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+SELECT count(*) FROM ft1;
+ count
+-------
+ 1000
+(1 row)
+
+FETCH c;
+ERROR: cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in
+ABORT;
+-- a cursor created in a released savepoint is handed to its parent
+BEGIN;
+SAVEPOINT s1;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+RELEASE s1;
+SAVEPOINT s2;
+SELECT count(*) FROM ft1;
+ count
+-------
+ 1000
+(1 row)
+
FETCH c;
ERROR: cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in
ABORT;
diff -ru a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
--- a/contrib/postgres_fdw/postgres_fdw.c 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/postgres_fdw.c 2026-10-03 05:55:27
@@ -190,7 +190,8 @@
FmgrInfo *param_flinfo; /* output conversion functions for them */
List *param_exprs; /* executable expressions for param values */
const char **param_values; /* textual values of query parameters */
- int created_at; /* xact depth at which the scan was created */
+ SubTransactionId created_subid; /* subxact in which the scan was created */
+ int created_level; /* its nesting depth at that time */
/* for storing result tuples */
HeapTuple *tuples; /* array of currently-retrieved tuples */
@@ -1765,8 +1766,9 @@
fsstate->cursor_number = GetCursorNumber(fsstate->conn);
fsstate->cursor_exists = false;
- /* Get the current local transaction's nesting depth */
- fsstate->created_at = GetCurrentTransactionNestLevel();
+ /* Remember the local (sub)transaction that the scan is created in */
+ fsstate->created_subid = GetCurrentSubTransactionId();
+ fsstate->created_level = GetCurrentTransactionNestLevel();
/* Get private info created by planner functions. */
fsstate->query = strVal(list_nth(fsplan->fdw_private,
@@ -4059,22 +4061,34 @@
StringInfoData buf;
PGresult *res;
- if (fsstate->created_at < GetCurrentTransactionNestLevel())
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in")));
+ /*
+ * The remote cursor is declared at the current remote (sub)transaction
+ * depth, and rolling back a remote savepoint at or below that depth
+ * destroys it. That is only safe if every local savepoint whose rollback
+ * keeps the local cursor alive is deeper than that. The local cursor
+ * lives at the depth of the (sub)transaction that created it, if that is
+ * still open; if it has been released, the cursor has been handed to some
+ * enclosing level, which we conservatively assume is the top level.
+ */
+ {
+ int cursor_level;
+ if (SubTransactionIsActive(fsstate->created_subid))
+ cursor_level = fsstate->created_level;
+ else
+ cursor_level = 1;
+
+ if (pgfdw_remote_xact_depth(fsstate->conn_state->entry) > cursor_level)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in")));
+ }
+
/* First, process a pending asynchronous request, if any. */
if (fsstate->conn_state->pendingAreq)
process_pending_request(fsstate->conn_state->pendingAreq);
/*
- * Second, synchronize the local/remote transactions. Note that we need
- * to do this because this function can be called from open cursors.
- */
- pgfdw_begin_remote_xact(fsstate->conn_state->entry);
-
- /*
* Construct array of query parameter values in text format. We do the
* conversions in the short-lived per-tuple context, so as not to cause a
* memory leak over repeated scans.
@@ -4116,6 +4130,15 @@
if (PQresultStatus(res) != PGRES_COMMAND_OK)
pgfdw_report_error(res, conn, fsstate->query);
PQclear(res);
+
+ /*
+ * Now synchronize the local/remote transactions. We do this after
+ * declaring the cursor, not before, so that the remote cursor is not
+ * created inside a remote savepoint that the local cursor doesn't
+ * belong to. (We need to synchronize here because this function can be
+ * called from open cursors.)
+ */
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
/* Mark the cursor as created, and show no tuples have been retrieved */
fsstate->cursor_exists = true;
diff -ru a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
--- a/contrib/postgres_fdw/postgres_fdw.h 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/postgres_fdw.h 2026-10-03 05:55:27
@@ -175,6 +175,7 @@
extern unsigned int GetPrepStmtNumber(PGconn *conn);
extern void do_sql_command(PGconn *conn, const char *sql);
extern void pgfdw_begin_remote_xact(struct ConnCacheEntry *entry);
+extern int pgfdw_remote_xact_depth(struct ConnCacheEntry *entry);
extern PGresult *pgfdw_get_result(PGconn *conn);
extern PGresult *pgfdw_exec_query(PGconn *conn, const char *query,
PgFdwConnState *state);
diff -ru a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql 2026-10-03 05:55:27
@@ -1652,9 +1652,31 @@
SELECT * FROM ft1 ORDER BY c1 LIMIT 1;
COMMIT;
+-- first fetch within a savepoint is fine if remote savepoint level is
+-- not advanced beyond the one the cursor was created in
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
SAVEPOINT s;
+FETCH c;
+ROLLBACK TO s;
+FETCH c;
+COMMIT;
+
+-- ... but not otherwise
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+SELECT count(*) FROM ft1;
+FETCH c;
+ABORT;
+
+-- a cursor created in a released savepoint is handed to its parent
+BEGIN;
+SAVEPOINT s1;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+RELEASE s1;
+SAVEPOINT s2;
+SELECT count(*) FROM ft1;
FETCH c;
ABORT;
Attachments:
[text/plain] v2-0001-Fix-open-cursor-handling.patch (9.8K, ../../DLV3PRA5KNY8.3ASR52GHRUYQ1@gmail.com/2-v2-0001-Fix-open-cursor-handling.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index b5d4cf3dccc..652fa4a943d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -397,7 +397,8 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user)
entry->mapping_hashvalue =
GetSysCacheHashValue1(USERMAPPINGOID,
ObjectIdGetDatum(user->umid));
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
+ entry->state.entry = entry;
/*
* Determine whether to keep the connection that we're about to make here
@@ -1061,6 +1062,20 @@ GetPrepStmtNumber(PGconn *conn)
return ++prep_stmt_number;
}
+/*
+ * Exported version of begin_remote_xact().
+ *
+ * This can be called for connections on which begin_remote_xact() has started
+ * a remote transaction.
+ */
+void
+pgfdw_begin_remote_xact(ConnCacheEntry *entry)
+{
+ Assert(entry);
+ Assert(entry->xact_depth > 0);
+ begin_remote_xact(entry);
+}
+
/*
* Submit a query and wait for the result.
*
@@ -1077,6 +1092,13 @@ pgfdw_exec_query(PGconn *conn, const char *query, PgFdwConnState *state)
if (state && state->pendingAreq)
process_pending_request(state->pendingAreq);
+ /*
+ * Second, synchronize the local/remote transactions. Note that we need
+ * to do this because this function can be called from open cursors.
+ */
+ if (state)
+ pgfdw_begin_remote_xact(state->entry);
+
if (!PQsendQuery(conn, query))
return NULL;
return pgfdw_get_result(conn);
@@ -1929,10 +1951,10 @@ pgfdw_abort_cleanup(ConnCacheEntry *entry, bool toplevel)
* If pendingAreq of the per-connection state is not NULL, it means that
* an asynchronous fetch begun by fetch_more_data_begin() was not done
* successfully and thus the per-connection state was not reset in
- * fetch_more_data(); in that case reset the per-connection state here.
+ * fetch_more_data(); in that case reset pendingAreq here.
*/
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* Disarm changing_xact_state if it all worked */
entry->changing_xact_state = false;
@@ -2221,9 +2243,9 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
entry->have_error = false;
}
- /* Reset the per-connection state if needed */
+ /* Reset pendingAreq here if any */
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* We're done with this entry; unset the changing_xact_state flag */
entry->changing_xact_state = false;
@@ -2266,9 +2288,9 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
entry->have_prep_stmt = false;
entry->have_error = false;
- /* Reset the per-connection state if needed */
+ /* Reset pendingAreq here if any */
if (entry->state.pendingAreq)
- memset(&entry->state, 0, sizeof(entry->state));
+ entry->state.pendingAreq = NULL;
/* We're done with this entry; unset the changing_xact_state flag */
entry->changing_xact_state = false;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 739f43af7bb..3aab56b0642 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -5332,6 +5332,8 @@ ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
+-- + cursors opened before a savepoint are disallowed to be first
+-- fetched within it
-- ===================================================================
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
@@ -5371,6 +5373,12 @@ SELECT * FROM ft1 ORDER BY c1 LIMIT 1;
(1 row)
COMMIT;
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+FETCH c;
+ERROR: cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in
+ABORT;
-- ===================================================================
-- test handling of collations
-- ===================================================================
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2bcff4b26b4..b1d75487c67 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -17,6 +17,7 @@
#include "access/htup_details.h"
#include "access/sysattr.h"
#include "access/table.h"
+#include "access/xact.h"
#include "catalog/pg_opfamily.h"
#include "commands/defrem.h"
#include "commands/explain_format.h"
@@ -189,6 +190,7 @@ typedef struct PgFdwScanState
FmgrInfo *param_flinfo; /* output conversion functions for them */
List *param_exprs; /* executable expressions for param values */
const char **param_values; /* textual values of query parameters */
+ int created_at; /* xact depth at which the scan was created */
/* for storing result tuples */
HeapTuple *tuples; /* array of currently-retrieved tuples */
@@ -1763,6 +1765,9 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
fsstate->cursor_number = GetCursorNumber(fsstate->conn);
fsstate->cursor_exists = false;
+ /* Get the current local transaction's nesting depth */
+ fsstate->created_at = GetCurrentTransactionNestLevel();
+
/* Get private info created by planner functions. */
fsstate->query = strVal(list_nth(fsplan->fdw_private,
FdwScanPrivateSelectSql));
@@ -4054,10 +4059,21 @@ create_cursor(ForeignScanState *node)
StringInfoData buf;
PGresult *res;
+ if (fsstate->created_at < GetCurrentTransactionNestLevel())
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in")));
+
/* First, process a pending asynchronous request, if any. */
if (fsstate->conn_state->pendingAreq)
process_pending_request(fsstate->conn_state->pendingAreq);
+ /*
+ * Second, synchronize the local/remote transactions. Note that we need
+ * to do this because this function can be called from open cursors.
+ */
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
+
/*
* Construct array of query parameter values in text format. We do the
* conversions in the short-lived per-tuple context, so as not to cause a
@@ -4147,7 +4163,7 @@ fetch_more_data(ForeignScanState *node)
if (PQresultStatus(res) != PGRES_TUPLES_OK)
pgfdw_report_error(res, conn, fsstate->query);
- /* Reset per-connection state */
+ /* Reset the pending asynchronous request */
fsstate->conn_state->pendingAreq = NULL;
}
else
@@ -4394,6 +4410,9 @@ create_foreign_modify(EState *estate,
* result if any. (This is the shared guts of postgresExecForeignInsert,
* postgresExecForeignBatchInsert, postgresExecForeignUpdate, and
* postgresExecForeignDelete.)
+ *
+ * Note: this function is never called from open cursors, thus no need to
+ * synchronize the local/remote transactions.
*/
static TupleTableSlot **
execute_foreign_modify(EState *estate,
@@ -4832,6 +4851,9 @@ rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist)
/*
* Execute a direct UPDATE/DELETE statement.
+ *
+ * Note: this function is never called from open cursors, thus no need to
+ * synchronize the local/remote transactions.
*/
static void
execute_dml_stmt(ForeignScanState *node)
@@ -8840,9 +8862,14 @@ fetch_more_data_begin(AsyncRequest *areq)
Assert(!fsstate->conn_state->pendingAreq);
- /* Create the cursor synchronously. */
+ /*
+ * Create the cursor synchronously if not already done. Otherwise,
+ * synchronize the local/remote transactions before the data fetch.
+ */
if (!fsstate->cursor_exists)
create_cursor(node);
+ else
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
/* We will send this query, but not wait for the response. */
snprintf(sql, sizeof(sql), "FETCH %d FROM c%u",
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index da7da1c2ea9..b9b460141f9 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -146,7 +146,8 @@ typedef struct PgFdwRelationInfo
*/
typedef struct PgFdwConnState
{
- AsyncRequest *pendingAreq; /* pending async request */
+ AsyncRequest *pendingAreq; /* pending async request */
+ struct ConnCacheEntry *entry; /* link to containing ConnCacheEntry */
} PgFdwConnState;
/*
@@ -173,6 +174,7 @@ extern void ReleaseConnection(PGconn *conn);
extern unsigned int GetCursorNumber(PGconn *conn);
extern unsigned int GetPrepStmtNumber(PGconn *conn);
extern void do_sql_command(PGconn *conn, const char *sql);
+extern void pgfdw_begin_remote_xact(struct ConnCacheEntry *entry);
extern PGresult *pgfdw_get_result(PGconn *conn);
extern PGresult *pgfdw_exec_query(PGconn *conn, const char *query,
PgFdwConnState *state);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index f1ca3204382..9c271953206 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1635,6 +1635,8 @@ ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
+-- + cursors opened before a savepoint are disallowed to be first
+-- fetched within it
-- ===================================================================
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
@@ -1650,6 +1652,12 @@ FETCH c;
SELECT * FROM ft1 ORDER BY c1 LIMIT 1;
COMMIT;
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+FETCH c;
+ABORT;
+
-- ===================================================================
-- test handling of collations
-- ===================================================================
[text/plain] v2-0002-Fix-xact-prop-issues.patch (5.9K, ../../DLV3PRA5KNY8.3ASR52GHRUYQ1@gmail.com/3-v2-0002-Fix-xact-prop-issues.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 652fa4a943d..77aec0fd9eb 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -112,6 +112,20 @@ static uint32 pgfdw_we_get_result = 0;
*/
#define RETRY_CANCEL_TIMEOUT 1000
+/*
+ * Macro for constructing commit command to be sent
+ *
+ * We synchronize the read/write mode before committing remote transactions
+ * so deferred triggers on remote servers can run in the right mode.
+ */
+#define CONSTRUCT_COMMIT_COMMAND(sql, entry) \
+ do { \
+ if ((read_only_level > 0) && !(entry)->xact_read_only) \
+ strcpy((sql), "SET TRANSACTION READ ONLY; COMMIT TRANSACTION"); \
+ else \
+ strcpy((sql), "COMMIT TRANSACTION"); \
+ } while(0)
+
/* Macro for constructing abort command to be sent */
#define CONSTRUCT_ABORT_COMMAND(sql, entry, toplevel) \
do { \
@@ -942,7 +956,7 @@ begin_remote_xact(ConnCacheEntry *entry)
appendStringInfoString(&sql, "REPEATABLE READ");
if (ro)
appendStringInfoString(&sql, " READ ONLY");
- if (XactDeferrable)
+ if (XactDeferrable && PQserverVersion(entry->conn) >= 90100)
appendStringInfoString(&sql, " DEFERRABLE");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
@@ -972,7 +986,7 @@ begin_remote_xact(ConnCacheEntry *entry)
if (entry->xact_depth == read_only_level)
{
entry->changing_xact_state = true;
- do_sql_command(entry->conn, "SET transaction_read_only = on");
+ do_sql_command(entry->conn, "SET TRANSACTION READ ONLY");
entry->xact_read_only = true;
entry->changing_xact_state = false;
}
@@ -1005,7 +1019,7 @@ begin_remote_xact(ConnCacheEntry *entry)
initStringInfo(&sql);
appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1);
if (ro)
- appendStringInfoString(&sql, "; SET transaction_read_only = on");
+ appendStringInfoString(&sql, "; SET TRANSACTION READ ONLY");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
entry->xact_depth++;
@@ -1206,6 +1220,24 @@ pgfdw_xact_callback(XactEvent event, void *arg)
if (!xact_got_connection)
return;
+ /*
+ * If we are called for pre-commit cleanup, ensure read_only_level is set
+ * for later processing. Note that we need to do this because the local
+ * transaction may have become read-only since the last remote operation.
+ */
+ if (event == XACT_EVENT_PARALLEL_PRE_COMMIT ||
+ event == XACT_EVENT_PRE_COMMIT)
+ {
+ if (XactReadOnly)
+ {
+ if (read_only_level == 0)
+ read_only_level = 1;
+ Assert(read_only_level == 1);
+ }
+ else
+ Assert(read_only_level == 0);
+ }
+
/*
* Scan all connection cache entries to find open remote transactions, and
* close them.
@@ -1222,6 +1254,8 @@ pgfdw_xact_callback(XactEvent event, void *arg)
/* If it has an open remote transaction, try to close it */
if (entry->xact_depth > 0)
{
+ char sql[100];
+
elog(DEBUG3, "closing remote transaction on connection %p",
entry->conn);
@@ -1237,14 +1271,17 @@ pgfdw_xact_callback(XactEvent event, void *arg)
pgfdw_reject_incomplete_xact_state_change(entry);
/* Commit all remote transactions during pre-commit */
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
entry->changing_xact_state = true;
if (entry->parallel_commit)
{
- do_sql_command_begin(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command_begin(entry->conn, sql);
pending_entries = lappend(pending_entries, entry);
continue;
}
- do_sql_command(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command(entry->conn, sql);
+ if ((read_only_level > 0) && !entry->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/*
@@ -2041,6 +2078,8 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
*/
foreach(lc, pending_entries)
{
+ char sql[100];
+
entry = (ConnCacheEntry *) lfirst(lc);
Assert(entry->changing_xact_state);
@@ -2049,7 +2088,10 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
* We might already have received the result on the socket, so pass
* consume_input=true to try to consume it first
*/
- do_sql_command_end(entry->conn, "COMMIT TRANSACTION", true);
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
+ do_sql_command_end(entry->conn, sql, true);
+ if ((read_only_level > 0) && !(entry)->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/* Do a DEALLOCATE ALL in parallel if needed */
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index fe4e6478e28..01577d8d69b 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -1148,20 +1148,17 @@ CREATE SUBSCRIPTION my_subscription SERVER subscription_server PUBLICATION testp
</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.)
+ Local <literal>READ ONLY</literal> transactions propagate their read-only
+ mode to remote sessions.
+ (This rule is also applied to 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.
+ Also, local <literal>DEFERRABLE</literal> transactions propagate their
+ deferrable mode to remote sessions.
+ (This rule is only applied to remote servers 9.1 and newer.)
</para>
<para>
[text/plain] v2-0003-Refine-open-cursor-handling.patch (7.5K, ../../DLV3PRA5KNY8.3ASR52GHRUYQ1@gmail.com/4-v2-0003-Refine-open-cursor-handling.patch)
download | inline diff:
diff -ru a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
--- a/contrib/postgres_fdw/connection.c 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/connection.c 2026-10-03 05:55:27
@@ -1091,6 +1091,16 @@
}
/*
+ * Return the nesting depth of the remote (sub)transaction currently open on
+ * the connection (0 if none).
+ */
+int
+pgfdw_remote_xact_depth(ConnCacheEntry *entry)
+{
+ return entry->xact_depth;
+}
+
+/*
* Submit a query and wait for the result.
*
* Since we don't use non-blocking mode, this can't process interrupts while
diff -ru a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
--- a/contrib/postgres_fdw/expected/postgres_fdw.out 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out 2026-10-03 05:55:27
@@ -5373,9 +5373,50 @@
(1 row)
COMMIT;
+-- first fetch within a savepoint is fine if remote savepoint level is
+-- not advanced beyond the one the cursor was created in
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
SAVEPOINT s;
+FETCH c;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
+(1 row)
+
+ROLLBACK TO s;
+FETCH c;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 2 | 2 | 00002 | Sat Jan 03 00:00:00 1970 PST | Sat Jan 03 00:00:00 1970 | 2 | 2 | foo
+(1 row)
+
+COMMIT;
+-- ... but not otherwise
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+SELECT count(*) FROM ft1;
+ count
+-------
+ 1000
+(1 row)
+
+FETCH c;
+ERROR: cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in
+ABORT;
+-- a cursor created in a released savepoint is handed to its parent
+BEGIN;
+SAVEPOINT s1;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+RELEASE s1;
+SAVEPOINT s2;
+SELECT count(*) FROM ft1;
+ count
+-------
+ 1000
+(1 row)
+
FETCH c;
ERROR: cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in
ABORT;
diff -ru a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
--- a/contrib/postgres_fdw/postgres_fdw.c 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/postgres_fdw.c 2026-10-03 05:55:27
@@ -190,7 +190,8 @@
FmgrInfo *param_flinfo; /* output conversion functions for them */
List *param_exprs; /* executable expressions for param values */
const char **param_values; /* textual values of query parameters */
- int created_at; /* xact depth at which the scan was created */
+ SubTransactionId created_subid; /* subxact in which the scan was created */
+ int created_level; /* its nesting depth at that time */
/* for storing result tuples */
HeapTuple *tuples; /* array of currently-retrieved tuples */
@@ -1765,8 +1766,9 @@
fsstate->cursor_number = GetCursorNumber(fsstate->conn);
fsstate->cursor_exists = false;
- /* Get the current local transaction's nesting depth */
- fsstate->created_at = GetCurrentTransactionNestLevel();
+ /* Remember the local (sub)transaction that the scan is created in */
+ fsstate->created_subid = GetCurrentSubTransactionId();
+ fsstate->created_level = GetCurrentTransactionNestLevel();
/* Get private info created by planner functions. */
fsstate->query = strVal(list_nth(fsplan->fdw_private,
@@ -4059,22 +4061,34 @@
StringInfoData buf;
PGresult *res;
- if (fsstate->created_at < GetCurrentTransactionNestLevel())
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in")));
+ /*
+ * The remote cursor is declared at the current remote (sub)transaction
+ * depth, and rolling back a remote savepoint at or below that depth
+ * destroys it. That is only safe if every local savepoint whose rollback
+ * keeps the local cursor alive is deeper than that. The local cursor
+ * lives at the depth of the (sub)transaction that created it, if that is
+ * still open; if it has been released, the cursor has been handed to some
+ * enclosing level, which we conservatively assume is the top level.
+ */
+ {
+ int cursor_level;
+ if (SubTransactionIsActive(fsstate->created_subid))
+ cursor_level = fsstate->created_level;
+ else
+ cursor_level = 1;
+
+ if (pgfdw_remote_xact_depth(fsstate->conn_state->entry) > cursor_level)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot perform the first fetch of a cursor within a deeper subtransaction than it was created in")));
+ }
+
/* First, process a pending asynchronous request, if any. */
if (fsstate->conn_state->pendingAreq)
process_pending_request(fsstate->conn_state->pendingAreq);
/*
- * Second, synchronize the local/remote transactions. Note that we need
- * to do this because this function can be called from open cursors.
- */
- pgfdw_begin_remote_xact(fsstate->conn_state->entry);
-
- /*
* Construct array of query parameter values in text format. We do the
* conversions in the short-lived per-tuple context, so as not to cause a
* memory leak over repeated scans.
@@ -4116,6 +4130,15 @@
if (PQresultStatus(res) != PGRES_COMMAND_OK)
pgfdw_report_error(res, conn, fsstate->query);
PQclear(res);
+
+ /*
+ * Now synchronize the local/remote transactions. We do this after
+ * declaring the cursor, not before, so that the remote cursor is not
+ * created inside a remote savepoint that the local cursor doesn't
+ * belong to. (We need to synchronize here because this function can be
+ * called from open cursors.)
+ */
+ pgfdw_begin_remote_xact(fsstate->conn_state->entry);
/* Mark the cursor as created, and show no tuples have been retrieved */
fsstate->cursor_exists = true;
diff -ru a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
--- a/contrib/postgres_fdw/postgres_fdw.h 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/postgres_fdw.h 2026-10-03 05:55:27
@@ -175,6 +175,7 @@
extern unsigned int GetPrepStmtNumber(PGconn *conn);
extern void do_sql_command(PGconn *conn, const char *sql);
extern void pgfdw_begin_remote_xact(struct ConnCacheEntry *entry);
+extern int pgfdw_remote_xact_depth(struct ConnCacheEntry *entry);
extern PGresult *pgfdw_get_result(PGconn *conn);
extern PGresult *pgfdw_exec_query(PGconn *conn, const char *query,
PgFdwConnState *state);
diff -ru a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql 2026-10-03 05:55:27
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql 2026-10-03 05:55:27
@@ -1652,9 +1652,31 @@
SELECT * FROM ft1 ORDER BY c1 LIMIT 1;
COMMIT;
+-- first fetch within a savepoint is fine if remote savepoint level is
+-- not advanced beyond the one the cursor was created in
BEGIN;
DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
SAVEPOINT s;
+FETCH c;
+ROLLBACK TO s;
+FETCH c;
+COMMIT;
+
+-- ... but not otherwise
+BEGIN;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+SAVEPOINT s;
+SELECT count(*) FROM ft1;
+FETCH c;
+ABORT;
+
+-- a cursor created in a released savepoint is handed to its parent
+BEGIN;
+SAVEPOINT s1;
+DECLARE c CURSOR FOR SELECT * FROM ft1 ORDER BY c1;
+RELEASE s1;
+SAVEPOINT s2;
+SELECT count(*) FROM ft1;
FETCH c;
ABORT;
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-03 12:04 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Matheus Alcantara <matheusssilv97@gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: Etsuro Fujita @ 2026-10-03 12:04 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: Nikolay Samokhvalov <nik@postgres.ai>; Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sat, Oct 3, 2026 at 6:38 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> On Fri Oct 2, 2026 at 12:54 PM -03, Etsuro Fujita wrote:
> > Here is an updated version for that. This is
> > an existing issue, so I split it into two:
> >
> > * v2-0001-Fix-open-cursor-handling.patch
> > This addresses the existing issue by disallowing the fetching (and the
> > issue #1 reported by Fujii-san as a side effect).
> >
> > * v2-0002-Fix-xact-prop-issues.patch
> > This addresses the remaining issues #2, #3 and #5 reported by
> > Fujii-san (#4 is not a bug). I will add test cases next.
>
> Thanks for the v2 patches! I tested them and the hot standby issue is
> fixed by 0002, and the deferred trigger case works as expected. I found
> two problems with 0001, 0002 looks good to me.
Thanks for reviewing!
I added the test cases to 0002. Attached is a new version of the
patch. (It actually doesn't depend on 0001, so I'm only attaching
0002.) I will push/backpatch the new version first if no objections
from others.
As for 0001, I also noticed problem #1 after sending the email...
0001 is an existing issue, so I will return to it, including your
patch, after closing this open item. Thanks for the patch!
Best regards,
Etsuro Fujita
Attachments:
[application/octet-stream] v3-0002-Fix-xact-prop-issues.patch (10.5K, ../../CAPmGK15okNxChu8wB_UwNWKTYbdjY=r3+7Wny3e_-bRrcaE+uA@mail.gmail.com/2-v3-0002-Fix-xact-prop-issues.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 652fa4a943d..77aec0fd9eb 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -112,6 +112,20 @@ static uint32 pgfdw_we_get_result = 0;
*/
#define RETRY_CANCEL_TIMEOUT 1000
+/*
+ * Macro for constructing commit command to be sent
+ *
+ * We synchronize the read/write mode before committing remote transactions
+ * so deferred triggers on remote servers can run in the right mode.
+ */
+#define CONSTRUCT_COMMIT_COMMAND(sql, entry) \
+ do { \
+ if ((read_only_level > 0) && !(entry)->xact_read_only) \
+ strcpy((sql), "SET TRANSACTION READ ONLY; COMMIT TRANSACTION"); \
+ else \
+ strcpy((sql), "COMMIT TRANSACTION"); \
+ } while(0)
+
/* Macro for constructing abort command to be sent */
#define CONSTRUCT_ABORT_COMMAND(sql, entry, toplevel) \
do { \
@@ -942,7 +956,7 @@ begin_remote_xact(ConnCacheEntry *entry)
appendStringInfoString(&sql, "REPEATABLE READ");
if (ro)
appendStringInfoString(&sql, " READ ONLY");
- if (XactDeferrable)
+ if (XactDeferrable && PQserverVersion(entry->conn) >= 90100)
appendStringInfoString(&sql, " DEFERRABLE");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
@@ -972,7 +986,7 @@ begin_remote_xact(ConnCacheEntry *entry)
if (entry->xact_depth == read_only_level)
{
entry->changing_xact_state = true;
- do_sql_command(entry->conn, "SET transaction_read_only = on");
+ do_sql_command(entry->conn, "SET TRANSACTION READ ONLY");
entry->xact_read_only = true;
entry->changing_xact_state = false;
}
@@ -1005,7 +1019,7 @@ begin_remote_xact(ConnCacheEntry *entry)
initStringInfo(&sql);
appendStringInfo(&sql, "SAVEPOINT s%d", entry->xact_depth + 1);
if (ro)
- appendStringInfoString(&sql, "; SET transaction_read_only = on");
+ appendStringInfoString(&sql, "; SET TRANSACTION READ ONLY");
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql.data);
entry->xact_depth++;
@@ -1206,6 +1220,24 @@ pgfdw_xact_callback(XactEvent event, void *arg)
if (!xact_got_connection)
return;
+ /*
+ * If we are called for pre-commit cleanup, ensure read_only_level is set
+ * for later processing. Note that we need to do this because the local
+ * transaction may have become read-only since the last remote operation.
+ */
+ if (event == XACT_EVENT_PARALLEL_PRE_COMMIT ||
+ event == XACT_EVENT_PRE_COMMIT)
+ {
+ if (XactReadOnly)
+ {
+ if (read_only_level == 0)
+ read_only_level = 1;
+ Assert(read_only_level == 1);
+ }
+ else
+ Assert(read_only_level == 0);
+ }
+
/*
* Scan all connection cache entries to find open remote transactions, and
* close them.
@@ -1222,6 +1254,8 @@ pgfdw_xact_callback(XactEvent event, void *arg)
/* If it has an open remote transaction, try to close it */
if (entry->xact_depth > 0)
{
+ char sql[100];
+
elog(DEBUG3, "closing remote transaction on connection %p",
entry->conn);
@@ -1237,14 +1271,17 @@ pgfdw_xact_callback(XactEvent event, void *arg)
pgfdw_reject_incomplete_xact_state_change(entry);
/* Commit all remote transactions during pre-commit */
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
entry->changing_xact_state = true;
if (entry->parallel_commit)
{
- do_sql_command_begin(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command_begin(entry->conn, sql);
pending_entries = lappend(pending_entries, entry);
continue;
}
- do_sql_command(entry->conn, "COMMIT TRANSACTION");
+ do_sql_command(entry->conn, sql);
+ if ((read_only_level > 0) && !entry->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/*
@@ -2041,6 +2078,8 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
*/
foreach(lc, pending_entries)
{
+ char sql[100];
+
entry = (ConnCacheEntry *) lfirst(lc);
Assert(entry->changing_xact_state);
@@ -2049,7 +2088,10 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
* We might already have received the result on the socket, so pass
* consume_input=true to try to consume it first
*/
- do_sql_command_end(entry->conn, "COMMIT TRANSACTION", true);
+ CONSTRUCT_COMMIT_COMMAND(sql, entry);
+ do_sql_command_end(entry->conn, sql, true);
+ if ((read_only_level > 0) && !(entry)->xact_read_only)
+ entry->xact_read_only = true;
entry->changing_xact_state = false;
/* Do a DEALLOCATE ALL in parallel if needed */
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 3aab56b0642..6c87a27243b 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -13373,6 +13373,7 @@ DROP VIEW my_application_name;
-- test read-only and/or deferrable transactions
-- ===================================================================
CREATE TABLE loct (f1 int, f2 text);
+INSERT INTO loct VALUES (1, 'foo'), (2, 'bar');
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;
@@ -13380,7 +13381,6 @@ 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
@@ -13469,9 +13469,32 @@ 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;
+-- Clean up
DROP FOREIGN TABLE remt;
+DROP FOREIGN TABLE remt2;
+DROP VIEW locv;
+DROP FUNCTION locf();
CREATE FOREIGN TABLE remt (f1 int, f2 text)
SERVER loopback OPTIONS (table_name 'loct');
+CREATE FUNCTION defer_trig_func() RETURNS TRIGGER LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.f2 IS NOT NULL THEN
+ UPDATE public.loct SET f2 = f2 || f2 WHERE f1 = NEW.f1;
+ END IF;
+ RETURN NULL;
+END;
+$$;
+CREATE CONSTRAINT TRIGGER defer_trig AFTER INSERT ON loct
+ DEFERRABLE INITIALLY DEFERRED
+ FOR EACH ROW EXECUTE PROCEDURE defer_trig_func();
+START TRANSACTION;
+INSERT INTO remt VALUES (3, 'baz');
+SET TRANSACTION READ ONLY;
+COMMIT;
+ERROR: cannot execute UPDATE in a read-only transaction
+CONTEXT: SQL statement "UPDATE public.loct SET f2 = f2 || f2 WHERE f1 = NEW.f1"
+PL/pgSQL function public.defer_trig_func() line 4 at SQL statement
+remote SQL command: SET TRANSACTION READ ONLY; COMMIT TRANSACTION
START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY;
SELECT * FROM remt;
f1 | f2
@@ -13501,9 +13524,8 @@ SELECT * FROM remt;
COMMIT;
-- Clean up
DROP FOREIGN TABLE remt;
-DROP FOREIGN TABLE remt2;
-DROP VIEW locv;
-DROP FUNCTION locf();
+DROP TRIGGER defer_trig ON loct;
+DROP FUNCTION defer_trig_func;
DROP TABLE loct;
-- ===================================================================
-- test parallel commit and parallel abort
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 9c271953206..0b313ebb418 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -4659,6 +4659,8 @@ DROP VIEW my_application_name;
-- test read-only and/or deferrable transactions
-- ===================================================================
CREATE TABLE loct (f1 int, f2 text);
+INSERT INTO loct VALUES (1, 'foo'), (2, 'bar');
+
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;
@@ -4666,7 +4668,6 @@ 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;
@@ -4712,10 +4713,32 @@ SET transaction_read_only = on;
SELECT * FROM remt2; -- should fail
ROLLBACK;
+-- Clean up
DROP FOREIGN TABLE remt;
+DROP FOREIGN TABLE remt2;
+DROP VIEW locv;
+DROP FUNCTION locf();
+
CREATE FOREIGN TABLE remt (f1 int, f2 text)
SERVER loopback OPTIONS (table_name 'loct');
+CREATE FUNCTION defer_trig_func() RETURNS TRIGGER LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.f2 IS NOT NULL THEN
+ UPDATE public.loct SET f2 = f2 || f2 WHERE f1 = NEW.f1;
+ END IF;
+ RETURN NULL;
+END;
+$$;
+CREATE CONSTRAINT TRIGGER defer_trig AFTER INSERT ON loct
+ DEFERRABLE INITIALLY DEFERRED
+ FOR EACH ROW EXECUTE PROCEDURE defer_trig_func();
+
+START TRANSACTION;
+INSERT INTO remt VALUES (3, 'baz');
+SET TRANSACTION READ ONLY;
+COMMIT;
+
START TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY;
SELECT * FROM remt;
COMMIT;
@@ -4730,9 +4753,8 @@ COMMIT;
-- Clean up
DROP FOREIGN TABLE remt;
-DROP FOREIGN TABLE remt2;
-DROP VIEW locv;
-DROP FUNCTION locf();
+DROP TRIGGER defer_trig ON loct;
+DROP FUNCTION defer_trig_func;
DROP TABLE loct;
-- ===================================================================
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index fe4e6478e28..01577d8d69b 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -1148,20 +1148,17 @@ CREATE SUBSCRIPTION my_subscription SERVER subscription_server PUBLICATION testp
</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.)
+ Local <literal>READ ONLY</literal> transactions propagate their read-only
+ mode to remote sessions.
+ (This rule is also applied to 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.
+ Also, local <literal>DEFERRABLE</literal> transactions propagate their
+ deferrable mode to remote sessions.
+ (This rule is only applied to remote servers 9.1 and newer.)
</para>
<para>
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-05 08:22 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Etsuro Fujita @ 2026-10-05 08:22 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: Nikolay Samokhvalov <nik@postgres.ai>; Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sat, Oct 3, 2026 at 9:04 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Sat, Oct 3, 2026 at 6:38 PM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
> > On Fri Oct 2, 2026 at 12:54 PM -03, Etsuro Fujita wrote:
> > > Here is an updated version for that. This is
> > > an existing issue, so I split it into two:
> > >
> > > * v2-0001-Fix-open-cursor-handling.patch
> > > This addresses the existing issue by disallowing the fetching (and the
> > > issue #1 reported by Fujii-san as a side effect).
> > >
> > > * v2-0002-Fix-xact-prop-issues.patch
> > > This addresses the remaining issues #2, #3 and #5 reported by
> > > Fujii-san (#4 is not a bug). I will add test cases next.
> >
> > Thanks for the v2 patches! I tested them and the hot standby issue is
> > fixed by 0002, and the deferred trigger case works as expected. I found
> > two problems with 0001, 0002 looks good to me.
> I added the test cases to 0002. Attached is a new version of the
> patch. (It actually doesn't depend on 0001, so I'm only attaching
> 0002.) I will push/backpatch the new version first if no objections
> from others.
Done.
> As for 0001, I also noticed problem #1 after sending the email...
> 0001 is an existing issue, so I will return to it, including your
> patch, after closing this open item.
I closed it and added the existing issue to Live issues.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-06 12:21 Matheus Alcantara <matheusssilv97@gmail.com>
parent: Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Matheus Alcantara @ 2026-10-06 12:21 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Nikolay Samokhvalov <nik@postgres.ai>; Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 05/10/26 05:22, Etsuro Fujita wrote:
> On Sat, Oct 3, 2026 at 9:04 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> On Sat, Oct 3, 2026 at 6:38 PM Matheus Alcantara
>> <matheusssilv97@gmail.com> wrote:
>>> On Fri Oct 2, 2026 at 12:54 PM -03, Etsuro Fujita wrote:
>>>> Here is an updated version for that. This is
>>>> an existing issue, so I split it into two:
>>>>
>>>> * v2-0001-Fix-open-cursor-handling.patch
>>>> This addresses the existing issue by disallowing the fetching (and the
>>>> issue #1 reported by Fujii-san as a side effect).
>>>>
>>>> * v2-0002-Fix-xact-prop-issues.patch
>>>> This addresses the remaining issues #2, #3 and #5 reported by
>>>> Fujii-san (#4 is not a bug). I will add test cases next.
>>>
>>> Thanks for the v2 patches! I tested them and the hot standby issue is
>>> fixed by 0002, and the deferred trigger case works as expected. I found
>>> two problems with 0001, 0002 looks good to me.
>
>> I added the test cases to 0002. Attached is a new version of the
>> patch. (It actually doesn't depend on 0001, so I'm only attaching
>> 0002.) I will push/backpatch the new version first if no objections
>> from others.
>
> Done.
>
>> As for 0001, I also noticed problem #1 after sending the email...
>> 0001 is an existing issue, so I will return to it, including your
>> patch, after closing this open item.
>
> I closed it and added the existing issue to Live issues.
>
Thank you!
Are you still planning to review 0001 and 0003 on this thread or
should I start a new one to discuss these patches?
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-06 22:37 Nikolay Samokhvalov <nik@postgres.ai>
parent: Matheus Alcantara <matheusssilv97@gmail.com>
1 sibling, 0 replies; 17+ messages in thread
From: Nikolay Samokhvalov @ 2026-10-06 22:37 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: Etsuro Fujita <etsuro.fujita@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sat, Oct 3, 2026 at 6:38 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> So 0003 declares the remote cursor before
> synchronizing the remote savepoint level, and raises the error only if
> the current remote depth is deeper than the level of the local cursor.
There is another case 0003 misses: parameter evaluation
can open a remote savepoint after create_cursor() checks the depth.
With 0001 and 0003 on REL_19_STABLE at a2148471, run this in a fresh
cursor_min database. Pass its socket directory and port as psql
variables sock and port:
create extension postgres_fdw;
create table t (id int);
insert into t values (1);
create server s foreign data wrapper postgres_fdw options (dbname
'cursor_min', host :'sock', port :'port');
create user mapping for current_user server s;
create foreign table ft (id int) server s options (table_name 't',
fetch_size '1');
create foreign table fa (id int) server s options (table_name 't');
begin;
declare c cursor for select * from ft where id = (select id from fa limit 1);
savepoint sp;
fetch c;
rollback to sp;
fetch c;
commit;
With 0001 alone, the first fetch is rejected by its depth check. With
0003, it returns 1, but the second fetch fails after rollback: cursor
"c2" does not exist. On clean REL_19_STABLE, the second fetch returns
no rows and commit succeeds.
The subquery becomes an initplan for the outer scan's $1 parameter.
process_query_params() runs it after the depth check. Its scan opens a
remote savepoint, so the outer cursor is declared inside it and removed
by rollback to sp.
Nik
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: postgres_fdw: transaction mode inheritance corner cases
@ 2026-10-07 02:50 Etsuro Fujita <etsuro.fujita@gmail.com>
parent: Matheus Alcantara <matheusssilv97@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Etsuro Fujita @ 2026-10-07 02:50 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: Nikolay Samokhvalov <nik@postgres.ai>; Fujii Masao <masao.fujii@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Oct 6, 2026 at 9:21 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> On 05/10/26 05:22, Etsuro Fujita wrote:
> >> As for 0001, I also noticed problem #1 after sending the email...
> >> 0001 is an existing issue, so I will return to it, including your
> >> patch, after closing this open item.
> >
> > I closed it and added the existing issue to Live issues.
> Are you still planning to review 0001 and 0003 on this thread or
> should I start a new one to discuss these patches?
Will do on this thread. It has been added to Live issues [1] ("Open
cursors querying postgres_fdw foreign tables not handled properly"),
so I think it's better to keep using it rather than creating a new
thread, for easier management.
Best regards,
Etsuro Fujita
[1] https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Live_issues
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2026-10-07 02:50 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-10 00:33 postgres_fdw: transaction mode inheritance corner cases Fujii Masao <masao.fujii@gmail.com>
2026-09-10 09:24 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-16 15:44 ` Nathan Bossart <nathandbossart@gmail.com>
2026-09-16 20:08 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-30 10:45 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-30 18:27 ` Matheus Alcantara <matheusssilv97@gmail.com>
2026-10-01 16:47 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-10-01 17:32 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-10-02 15:54 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-10-03 09:38 ` Matheus Alcantara <matheusssilv97@gmail.com>
2026-10-03 12:04 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-10-05 08:22 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-10-06 12:21 ` Matheus Alcantara <matheusssilv97@gmail.com>
2026-10-07 02:50 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-10-06 22:37 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-10-01 17:30 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-10-01 17:37 ` Etsuro Fujita <etsuro.fujita@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox