Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. postgres_fdw: Fix handling of abort-cleanup-failed connections.

  1. Use-after-free issue in postgres_fdw

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-19T14:56:13Z

    Hi,

I got an offline report from my colleague Zhibai Song that
close_cursor() is called for a freed PGconn, leading to a server
crash.  Here is a reproducer (the original reproducer he provided is a
bit complex, so I simplified it):

create server loopback
    foreign data wrapper postgres_fdw
    options (dbname 'postgres');
create user mapping for current_user server loopback;
create table t1 (id int, data text);
create foreign table ft1 (id int, data text)
    server loopback options (table_name 't1');
insert into ft1 values (1, 'foo');
start transaction;
-- This caches the remote connection's PGconn in PgFdwScanState
declare c1 cursor for select * from ft1;
fetch c1;
 id | data
----+------
  1 | foo
(1 row)

savepoint s1;
select * from ft1;
 id | data
----+------
  1 | foo
(1 row)

select pid from pg_stat_activity
    where datname = 'postgres'
      and application_name = 'postgres_fdw';
  pid
-------
 91853
(1 row)

-- This terminates the remote session
select pg_terminate_backend(91853);
 pg_terminate_backend
----------------------
 t
(1 row)

-- This leaves the remote connection's changing_xact_state as true
rollback to s1;
savepoint s1;
-- This calls pgfdw_reject_incomplete_xact_state_change(), freeing
-- the remote connection's PGconn as changing_xact_state is true
select * from ft1;
ERROR:  connection to server "loopback" was lost
rollback to s1;
-- This calls close_cursor() on the PGconn cached in PgFdwScanState,
-- which was freed above, leading to a server crash
close c1;

I think the root cause is that it is too early to free the PGconn in
pgfdw_reject_incomplete_xact_state_change() even if the connection is
in a state where we cannot use it any further; I think we should delay
that until abort cleanup (ie, pgfdw_xact_callback()).  Attached is a
patch for that.

Best regards,
Etsuro Fujita

  2. Re: Use-after-free issue in postgres_fdw

    Chao Li <li.evan.chao@gmail.com> — 2026-03-20T08:27:21Z

    
> On Mar 19, 2026, at 22:56, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> 
> Hi,
> 
> I got an offline report from my colleague Zhibai Song that
> close_cursor() is called for a freed PGconn, leading to a server
> crash.  Here is a reproducer (the original reproducer he provided is a
> bit complex, so I simplified it):
> 
> create server loopback
>    foreign data wrapper postgres_fdw
>    options (dbname 'postgres');
> create user mapping for current_user server loopback;
> create table t1 (id int, data text);
> create foreign table ft1 (id int, data text)
>    server loopback options (table_name 't1');
> insert into ft1 values (1, 'foo');
> start transaction;
> -- This caches the remote connection's PGconn in PgFdwScanState
> declare c1 cursor for select * from ft1;
> fetch c1;
> id | data
> ----+------
>  1 | foo
> (1 row)
> 
> savepoint s1;
> select * from ft1;
> id | data
> ----+------
>  1 | foo
> (1 row)
> 
> select pid from pg_stat_activity
>    where datname = 'postgres'
>      and application_name = 'postgres_fdw';
>  pid
> -------
> 91853
> (1 row)
> 
> -- This terminates the remote session
> select pg_terminate_backend(91853);
> pg_terminate_backend
> ----------------------
> t
> (1 row)
> 
> -- This leaves the remote connection's changing_xact_state as true
> rollback to s1;
> savepoint s1;
> -- This calls pgfdw_reject_incomplete_xact_state_change(), freeing
> -- the remote connection's PGconn as changing_xact_state is true
> select * from ft1;
> ERROR:  connection to server "loopback" was lost
> rollback to s1;
> -- This calls close_cursor() on the PGconn cached in PgFdwScanState,
> -- which was freed above, leading to a server crash
> close c1;
> 
> I think the root cause is that it is too early to free the PGconn in
> pgfdw_reject_incomplete_xact_state_change() even if the connection is
> in a state where we cannot use it any further; I think we should delay
> that until abort cleanup (ie, pgfdw_xact_callback()).  Attached is a
> patch for that.
> 
> Best regards,
> Etsuro Fujita
> <fix-connection-handling-in-postgres-fdw.patch>

Hi Etsuro-san,

I can reproduce the server crash following your procedure, and I traced the problem.

The issue is that, during select * from ft1, pgfdw_reject_incomplete_xact_state_change() calls disconnect_pg_server(), which destroys conn and sets ConnCacheEntry->conn = NULL, but does not update PgFdwScanState->conn. As a result, when "close c1" is executed later, PgFdwScanState->conn points to stale memory with random contents.

I am not sure we should still allow further commands to run after select * from ft1, given that it has already raised: "ERROR:  connection to server "loopback" was lost”. Maybe we should not keep going as if the connection were still there.

I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss. My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  3. Re: Use-after-free issue in postgres_fdw

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-03-20T16:00:48Z

    Hi,

On 19/03/26 11:56, Etsuro Fujita wrote:
> I think the root cause is that it is too early to free the PGconn in
> pgfdw_reject_incomplete_xact_state_change() even if the connection is
> in a state where we cannot use it any further; I think we should delay
> that until abort cleanup (ie, pgfdw_xact_callback()).  Attached is a
> patch for that.
> 

IIUC, with this patch, disconnect_pg_server() will be called at 
pgfdw_xact_callback() via pgfdw_reset_xact_state() when the top level 
transaction is rollback right?

I've tested and it seems to fix the issue, when "close c1;" is 
executed "conn" points to a valid connection pointer and this 
connection is properly disconnected when the top level transactions is 
rollback.

This issue is reproducible on v14, so I think that we need a back-port.

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

  4. Re: Use-after-free issue in postgres_fdw

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-03-20T16:14:45Z

    On 20/03/26 05:27, Chao Li wrote:
> I can reproduce the server crash following your procedure, and I traced the problem.
> 
> The issue is that, during select * from ft1, pgfdw_reject_incomplete_xact_state_change() calls disconnect_pg_server(), which destroys conn and sets ConnCacheEntry->conn = NULL, but does not update PgFdwScanState->conn. As a result, when "close c1" is executed later, PgFdwScanState->conn points to stale memory with random contents.
> 
> I am not sure we should still allow further commands to run after select * from ft1, given that it has already raised: "ERROR:  connection to server "loopback" was lost”. Maybe we should not keep going as if the connection were still there.
> 
> I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss. My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.
> 

Although I agree with this I think that it will be a quite invasive 
change to fix this issue, considering that it should be back-ported.

I see two ways to implement this:

1. ForeignScanState->conn points to a ConnCacheEntry and it access the 
PGConn via ConnCacheEntry->conn, so it can check if != NULL before using.

2. Make pgfdw_reject_incomplete_xact_state_change() or 
disconnect_pg_server() receive a PgFdwConnState and add a new field on 
this state to represent that the connection is closed and them check 
this field on the proper code paths.

With the patch proposed on the previous email the server don't crash 
and any use of PgFdwScanState->conn will make the command to fail with 
something like this:

    ERROR:  08006: no connection to the server
    CONTEXT:  remote SQL command: CLOSE c1
    LOCATION:  pgfdw_report_internal, connection.c:1059


So I don't think that this change would be worth, or I'm missing 
something? What do you think?

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

  5. Re: Use-after-free issue in postgres_fdw

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-21T11:40:50Z

    Hi Chao,

On Fri, Mar 20, 2026 at 5:27 PM Chao Li <li.evan.chao@gmail.com> wrote:
> I can reproduce the server crash following your procedure, and I traced the problem.
>
> The issue is that, during select * from ft1, pgfdw_reject_incomplete_xact_state_change() calls disconnect_pg_server(), which destroys conn and sets ConnCacheEntry->conn = NULL, but does not update PgFdwScanState->conn. As a result, when "close c1" is executed later, PgFdwScanState->conn points to stale memory with random contents.

That is right.

> I am not sure we should still allow further commands to run after select * from ft1, given that it has already raised: "ERROR:  connection to server "loopback" was lost”. Maybe we should not keep going as if the connection were still there.

The ERROR is thrown within a subtransaction, not within the main
transaction, so we *should* allow commands (that don't use the
connection) to run *after* rolling back to the subtransaction.

(A transaction, like the example one, whose subtransactions failed
abort cleanup due to a connection issue needs to abort in the end, as
it can't commit the remote transaction anymore.  Considering this
limitation, the above behavior might be surprising to users.  We might
need to do something about this, but I think that that is another
issue and thus should be discussed separately.)

> I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss.

Why?  While we can't use the connection any further, with the patch we
can still use the cashed PGconn, as pointed out by Matheus downthread.

> My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.

I don't think that that is a good idea, because that 1) would require
invasive changes to the existing code, which isn't good especially for
back-patching, as also mentioned by Matheus downthread, and 2) would
result in something that is essentially the same as what I proposed;
in other words, that would be a lot of effort with little result.

Thanks for looking at this!

Best regards,
Etsuro Fujita

  6. Re: Use-after-free issue in postgres_fdw

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-21T11:44:47Z

    Hi Matheus,

On Sat, Mar 21, 2026 at 1:00 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> On 19/03/26 11:56, Etsuro Fujita wrote:
> > I think the root cause is that it is too early to free the PGconn in
> > pgfdw_reject_incomplete_xact_state_change() even if the connection is
> > in a state where we cannot use it any further; I think we should delay
> > that until abort cleanup (ie, pgfdw_xact_callback()).  Attached is a
> > patch for that.
>
> IIUC, with this patch, disconnect_pg_server() will be called at
> pgfdw_xact_callback() via pgfdw_reset_xact_state() when the top level
> transaction is rollback right?

That is right.

> I've tested and it seems to fix the issue, when "close c1;" is
> executed "conn" points to a valid connection pointer and this
> connection is properly disconnected when the top level transactions is
> rollback.
>
> This issue is reproducible on v14, so I think that we need a back-port.

Yes, I am thinking of back-patching the fix to all supported versions,
if no objections.

Thanks for testing/reviewing the patch!

Best regards,
Etsuro Fujita

  7. Re: Use-after-free issue in postgres_fdw

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-03-21T11:56:41Z

    On Sat, Mar 21, 2026 at 1:14 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> On 20/03/26 05:27, Chao Li wrote:
> > I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss. My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.
>
> Although I agree with this I think that it will be a quite invasive
> change to fix this issue, considering that it should be back-ported.
>
> I see two ways to implement this:
>
> 1. ForeignScanState->conn points to a ConnCacheEntry and it access the
> PGConn via ConnCacheEntry->conn, so it can check if != NULL before using.
>
> 2. Make pgfdw_reject_incomplete_xact_state_change() or
> disconnect_pg_server() receive a PgFdwConnState and add a new field on
> this state to represent that the connection is closed and them check
> this field on the proper code paths.
>
> With the patch proposed on the previous email the server don't crash
> and any use of PgFdwScanState->conn will make the command to fail with
> something like this:
>
>     ERROR:  08006: no connection to the server
>     CONTEXT:  remote SQL command: CLOSE c1
>     LOCATION:  pgfdw_report_internal, connection.c:1059
>
>
> So I don't think that this change would be worth, or I'm missing
> something? What do you think?

I don't feel the need to do these, either, for the reason mentioned upthread.

Thanks again!

Best regards,
Etsuro Fujita

  8. Re: Use-after-free issue in postgres_fdw

    Chao Li <li.evan.chao@gmail.com> — 2026-03-23T02:48:25Z

    
> On Mar 21, 2026, at 19:40, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> 
> Hi Chao,
> 
> On Fri, Mar 20, 2026 at 5:27 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> I can reproduce the server crash following your procedure, and I traced the problem.
>> 
>> The issue is that, during select * from ft1, pgfdw_reject_incomplete_xact_state_change() calls disconnect_pg_server(), which destroys conn and sets ConnCacheEntry->conn = NULL, but does not update PgFdwScanState->conn. As a result, when "close c1" is executed later, PgFdwScanState->conn points to stale memory with random contents.
> 
> That is right.
> 
>> I am not sure we should still allow further commands to run after select * from ft1, given that it has already raised: "ERROR:  connection to server "loopback" was lost”. Maybe we should not keep going as if the connection were still there.
> 
> The ERROR is thrown within a subtransaction, not within the main
> transaction, so we *should* allow commands (that don't use the
> connection) to run *after* rolling back to the subtransaction.
> 
> (A transaction, like the example one, whose subtransactions failed
> abort cleanup due to a connection issue needs to abort in the end, as
> it can't commit the remote transaction anymore.  Considering this
> limitation, the above behavior might be surprising to users.  We might
> need to do something about this, but I think that that is another
> issue and thus should be discussed separately.)

Thanks for the explanation. Makes sense to me.

> 
>> I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss.
> 
> Why?  While we can't use the connection any further, with the patch we
> can still use the cashed PGconn, as pointed out by Matheus downthread.
> 
>> My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.
> 
> I don't think that that is a good idea, because that 1) would require
> invasive changes to the existing code, which isn't good especially for
> back-patching, as also mentioned by Matheus downthread, and 2) would
> result in something that is essentially the same as what I proposed;
> in other words, that would be a lot of effort with little result.
> 

Makes sense. Then the patch looks good to me.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  9. Re: Use-after-free issue in postgres_fdw

    Michael Paquier <michael@paquier.xyz> — 2026-03-24T08:06:38Z

    On Thu, Mar 19, 2026 at 11:56:13PM +0900, Etsuro Fujita wrote:
> I got an offline report from my colleague Zhibai Song that
> close_cursor() is called for a freed PGconn, leading to a server
> crash.  Here is a reproducer (the original reproducer he provided is a
> bit complex, so I simplified it):

Hmm.  We have one isolation test with a pg_terminate_backend() that
checks for a backend terminating, see temp-schema-cleanup.

Do you think that it would be worth having a test for this scenario,
where we could terminate a remote connection?  An isolation test may
work, I hope, providing some insurance with the order of the
operations able to trigger the use-after-free behavior.

> I think the root cause is that it is too early to free the PGconn in
> pgfdw_reject_incomplete_xact_state_change() even if the connection is
> in a state where we cannot use it any further; I think we should delay
> that until abort cleanup (ie, pgfdw_xact_callback()).  Attached is a
> patch for that.

Removing the early disconnect() means that we should try to cover our
tracks in all the existing ,code paths where we finish the
top-transaction.  Looking at the locations of
pgfdw_reset_xact_state(), it looks like you are getting yourself
covered.
--
Michael

  10. Re: Use-after-free issue in postgres_fdw

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-04-24T08:59:22Z

    On Tue, Mar 24, 2026 at 5:06 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Mar 19, 2026 at 11:56:13PM +0900, Etsuro Fujita wrote:
> > I got an offline report from my colleague Zhibai Song that
> > close_cursor() is called for a freed PGconn, leading to a server
> > crash.  Here is a reproducer (the original reproducer he provided is a
> > bit complex, so I simplified it):
>
> Hmm.  We have one isolation test with a pg_terminate_backend() that
> checks for a backend terminating, see temp-schema-cleanup.
>
> Do you think that it would be worth having a test for this scenario,
> where we could terminate a remote connection?  An isolation test may
> work, I hope, providing some insurance with the order of the
> operations able to trigger the use-after-free behavior.

I added a simple SQL test, not an isolation one.  Attached is an
updated patch for that.  The basic idea for the test is the same as
the one I showed upthread, but the test ensures the order by checking
for the termination by using pg_stat_activity.  In the patch I added a
comment for pgfdw_reject_incomplete_xact_state_change() as well.

> > I think the root cause is that it is too early to free the PGconn in
> > pgfdw_reject_incomplete_xact_state_change() even if the connection is
> > in a state where we cannot use it any further; I think we should delay
> > that until abort cleanup (ie, pgfdw_xact_callback()).  Attached is a
> > patch for that.
>
> Removing the early disconnect() means that we should try to cover our
> tracks in all the existing ,code paths where we finish the
> top-transaction.  Looking at the locations of
> pgfdw_reset_xact_state(), it looks like you are getting yourself
> covered.

Right, this patch delays discarding the failed connection until abort
cleanup of the top transaction, so it is guaranteed that anyone that
uses it can safely refer to its PGconn when needed.

Thanks for the comments!  Sorry for the delay.

Best regards,
Etsuro Fujita

  11. Re: Use-after-free issue in postgres_fdw

    Michael Paquier <michael@paquier.xyz> — 2026-04-27T08:07:02Z

    On Fri, Apr 24, 2026 at 01:59:22AM -0700, Etsuro Fujita wrote:
> I added a simple SQL test, not an isolation one.  Attached is an
> updated patch for that.  The basic idea for the test is the same as
> the one I showed upthread, but the test ensures the order by checking
> for the termination by using pg_stat_activity.  In the patch I added a
> comment for pgfdw_reject_incomplete_xact_state_change() as well.

Smart.

> Right, this patch delays discarding the failed connection until abort
> cleanup of the top transaction, so it is guaranteed that anyone that
> uses it can safely refer to its PGconn when needed.

+-- test handling of abort-cleanup-failed connections

Perhaps this could be more descriptive, like "test cleanup of failed
connections on abort"?
--
Michael

  12. Re: Use-after-free issue in postgres_fdw

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-05-01T12:05:57Z

    On Mon, Apr 27, 2026 at 5:07 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Apr 24, 2026 at 01:59:22AM -0700, Etsuro Fujita wrote:
> > Right, this patch delays discarding the failed connection until abort
> > cleanup of the top transaction, so it is guaranteed that anyone that
> > uses it can safely refer to its PGconn when needed.
>
> +-- test handling of abort-cleanup-failed connections
>
> Perhaps this could be more descriptive, like "test cleanup of failed
> connections on abort"?

Sounds better.  I'll modify the comment as such, and if no objections,
push an updated version and back-patch it to all supported versions.

Thanks for the comments!

Best regards,
Etsuro Fujita

  13. Re: Use-after-free issue in postgres_fdw

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-05-05T10:19:54Z

    On Fri, May 1, 2026 at 9:05 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Mon, Apr 27, 2026 at 5:07 PM Michael Paquier <michael@paquier.xyz> wrote:
> > On Fri, Apr 24, 2026 at 01:59:22AM -0700, Etsuro Fujita wrote:
> > > Right, this patch delays discarding the failed connection until abort
> > > cleanup of the top transaction, so it is guaranteed that anyone that
> > > uses it can safely refer to its PGconn when needed.
> >
> > +-- test handling of abort-cleanup-failed connections
> >
> > Perhaps this could be more descriptive, like "test cleanup of failed
> > connections on abort"?
>
> Sounds better.  I'll modify the comment as such, and if no objections,
> push an updated version and back-patch it to all supported versions.

Pushed and backpatched after modifying the comment as proposed,
tweaking a code comment a bit, and adding a variant of a test case.

Best regards,
Etsuro Fujita