Thread: Options to control remote transactions’ access/deferrable modes in postgres_fdw

Hi,

postgres_fdw opens remote transactions in read/write mode in a local
transaction even if the local transaction is read-only.  I noticed
that this leads to surprising behavior like this:

CREATE TABLE test (a int);
CREATE FUNCTION testfunc() RETURNS int LANGUAGE SQL AS 'INSERT INTO
public.test VALUES (1) RETURNING *';
CREATE VIEW testview(a) AS SELECT testfunc();
CREATE FOREIGN TABLE testft (a int) SERVER loopback OPTIONS
(table_name 'testview');

START TRANSACTION READ ONLY;
SELECT * FROM testft;
 a
---
 1
(1 row)

COMMIT;
SELECT * FROM test;
 a
---
 1
(1 row)

The transaction is declared as READ ONLY, but the INSERT statement is
successfully executed in the remote side.

To avoid that, I would like to propose a server option,
inherit_read_only, to open the remote transactions in read-only mode
if the local transaction is read-only.

I would also like to propose a server option, inherit_deferrable, to
open the remote transactions in deferrable mode if the local
transaction is deferrable.

Attached is a small patch for these options.  I will add this to the
March commitfest as it is still open.

Best regards,
Etsuro Fujita

On Sun, Mar 2, 2025 at 12:44 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> Attached is a small patch for these options.  I will add this to the
> March commitfest as it is still open.

The CF was changed to in-progress just before, so I added it to the next CF.

Best regards,
Etsuro Fujita

Hi Fujita-san,


On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> Hi,
>
> postgres_fdw opens remote transactions in read/write mode in a local
> transaction even if the local transaction is read-only.  I noticed
> that this leads to surprising behavior like this:
>
> CREATE TABLE test (a int);
> CREATE FUNCTION testfunc() RETURNS int LANGUAGE SQL AS 'INSERT INTO
> public.test VALUES (1) RETURNING *';
> CREATE VIEW testview(a) AS SELECT testfunc();
> CREATE FOREIGN TABLE testft (a int) SERVER loopback OPTIONS
> (table_name 'testview');
>
> START TRANSACTION READ ONLY;
> SELECT * FROM testft;
>  a
> ---
>  1
> (1 row)
>
> COMMIT;
> SELECT * FROM test;
>  a
> ---
>  1
> (1 row)

I am having a hard time deciding whether this is problematic behaviour
or not. Maybe the way example is setup - it's querying a view on a
remote database which doesn't return anything but modified data. If
there is no modification happening on the foreign server it won't
return any data. Thus we have no way to verify that the table changed
because of a READ ONLY transaction which is not expected to change any
data. Probably some other example which returns all the rows from test
while modifying some of it might be better.

>
> The transaction is declared as READ ONLY, but the INSERT statement is
> successfully executed in the remote side.
>
> To avoid that, I would like to propose a server option,
> inherit_read_only, to open the remote transactions in read-only mode
> if the local transaction is read-only.

Why do we need a server option. Either we say that a local READ ONLY
transaction causing modifications on the foreign server is problematic
or it's expected. But what's the point in giving that choice to the
user? If we deem the behaviour problematic it should be considered as
a bug and we should fix it. Otherwise not fix it.

>
> I would also like to propose a server option, inherit_deferrable, to
> open the remote transactions in deferrable mode if the local
> transaction is deferrable.

The documentation about deferrable is quite confusing. It says "The
DEFERRABLE transaction property has no effect unless the transaction
is also SERIALIZABLE and READ ONLY." But it doesn't tell what's the
effect of deferrable transaction. But probably we don't need a server
option here as well.

--
Best Wishes,
Ashutosh Bapat

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> To avoid that, I would like to propose a server option,
>> inherit_read_only, to open the remote transactions in read-only mode
>> if the local transaction is read-only.

> Why do we need a server option. Either we say that a local READ ONLY
> transaction causing modifications on the foreign server is problematic
> or it's expected. But what's the point in giving that choice to the
> user? If we deem the behaviour problematic it should be considered as
> a bug and we should fix it. Otherwise not fix it.

I tend to agree with Ashutosh's position here.  Reasoning about
issues like this is hard enough already.  Having to figure out an
application's behavior under more than one setting makes it harder.
You may argue that "then the application can choose the behavior it
likes, so there's no need to figure out both behaviors".  But for a
lot of bits of code, that's not the situation; rather, they have to
be prepared to work under both settings, because someone else is
in charge of what the setting is.  (I don't know if either of you
recall our disastrous attempt at server-side autocommit, back around
7.3.  The reason that got reverted was exactly that there was too
much code that had to be prepared to work under either setting,
and it was too hard to make that happen.  So now I look with great
suspicion at anything that complicates our transactional behavior.)

>> I would also like to propose a server option, inherit_deferrable, to
>> open the remote transactions in deferrable mode if the local
>> transaction is deferrable.

> The documentation about deferrable is quite confusing. It says "The
> DEFERRABLE transaction property has no effect unless the transaction
> is also SERIALIZABLE and READ ONLY." But it doesn't tell what's the
> effect of deferrable transaction. But probably we don't need a server
> option here as well.

Yeah, same with this: we should either change it or not.  Multiple
possible transactional behaviors don't do anyone any favors.

            regards, tom lane

On Mon, Mar 3, 2025 at 4:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> > On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> >> To avoid that, I would like to propose a server option,
> >> inherit_read_only, to open the remote transactions in read-only mode
> >> if the local transaction is read-only.
>
> > Why do we need a server option. Either we say that a local READ ONLY
> > transaction causing modifications on the foreign server is problematic
> > or it's expected. But what's the point in giving that choice to the
> > user? If we deem the behaviour problematic it should be considered as
> > a bug and we should fix it. Otherwise not fix it.
>
> I tend to agree with Ashutosh's position here.  Reasoning about
> issues like this is hard enough already.  Having to figure out an
> application's behavior under more than one setting makes it harder.
> You may argue that "then the application can choose the behavior it
> likes, so there's no need to figure out both behaviors".  But for a
> lot of bits of code, that's not the situation; rather, they have to
> be prepared to work under both settings, because someone else is
> in charge of what the setting is.  (I don't know if either of you
> recall our disastrous attempt at server-side autocommit, back around
> 7.3.  The reason that got reverted was exactly that there was too
> much code that had to be prepared to work under either setting,
> and it was too hard to make that happen.  So now I look with great
> suspicion at anything that complicates our transactional behavior.)
>
> >> I would also like to propose a server option, inherit_deferrable, to
> >> open the remote transactions in deferrable mode if the local
> >> transaction is deferrable.
>
> > The documentation about deferrable is quite confusing. It says "The
> > DEFERRABLE transaction property has no effect unless the transaction
> > is also SERIALIZABLE and READ ONLY." But it doesn't tell what's the
> > effect of deferrable transaction. But probably we don't need a server
> > option here as well.
>
> Yeah, same with this: we should either change it or not.  Multiple
> possible transactional behaviors don't do anyone any favors.

I thought some users might rely on the current behavior that remote
transactions can write even if the local transaction is READ ONLY, so
it would be a good idea to add a server option for backwards
compatibility, but I agree that such an option would cause another,
more difficult problem you mentioned above.

I think the current behavior is not easy to understand, causing
unexpected results in a READ ONLY transaction as shown upthread, so I
would like to instead propose to fix it (for HEAD only as there are no
reports on this issue if I remember correctly).  I think those relying
on the current behavior should just re-declare their transactions as
READ WRITE.  Attached is an updated version of the patch, which fixes
the deferrable case as well.

In the patch I also fixed a bug; I trusted XactReadOnly to see if the
local transaction is READ ONLY, but I noticed that that is not 100%
correct, because a transaction which started as READ WRITE can show as
READ ONLY later within subtransactions, so I modified the patch so
that postgres_fdw opens remote transactions in READ ONLY mode if the
local transaction has been declared READ ONLY at the top level.

Thank you both!

Best regards,
Etsuro Fujita

On Mon, Mar 3, 2025 at 1:51 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > postgres_fdw opens remote transactions in read/write mode in a local
> > transaction even if the local transaction is read-only.  I noticed
> > that this leads to surprising behavior like this:

> I am having a hard time deciding whether this is problematic behaviour
> or not. Maybe the way example is setup - it's querying a view on a
> remote database which doesn't return anything but modified data. If
> there is no modification happening on the foreign server it won't
> return any data. Thus we have no way to verify that the table changed
> because of a READ ONLY transaction which is not expected to change any
> data. Probably some other example which returns all the rows from test
> while modifying some of it might be better.

How about something like this?

CREATE TABLE loct (f1 int, f2 text);
CREATE FUNCTION locf() RETURNS SETOF loct LANGUAGE SQL AS
  'UPDATE public.loct SET f2 = f2 || f2 RETURNING *';
CREATE VIEW locv AS SELECT t.* FROM locf() t;
CREATE FOREIGN TABLE remt (f1 int, f2 text)
  SERVER loopback OPTIONS (table_name 'locv');
INSERT INTO loct VALUES (1, 'foo'), (2, 'bar');
SELECT * FROM loct;
 f1 | f2
----+-----
  1 | foo
  2 | bar
(2 rows)

SELECT * FROM remt;  -- should work
 f1 |   f2
----+--------
  1 | foofoo
  2 | barbar
(2 rows)

SELECT * FROM loct;
 f1 |   f2
----+--------
  1 | foofoo
  2 | barbar
(2 rows)

I added this test case to the updated patch [1].

Thanks for the comments!

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/CAPmGK14Btk0odkH6vwBhBGjCexmmWcM_D3DG0pJtObj8k_Unag%40mail.gmail.com

On Tue, Mar 25, 2025 at 4:01 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> In the patch I also fixed a bug; I trusted XactReadOnly to see if the
> local transaction is READ ONLY, but I noticed that that is not 100%
> correct, because a transaction which started as READ WRITE can show as
> READ ONLY later within subtransactions, so I modified the patch so
> that postgres_fdw opens remote transactions in READ ONLY mode if the
> local transaction has been declared READ ONLY at the top level.

Nice catch. postgres_fdw replicates the transaction stack on foreign
server. I think we need to replicate it along with the transaction
properties. And also we need a test which tests readonly
subtransaction behaviour.

--
Best Wishes,
Ashutosh Bapat

On Thu, Mar 27, 2025 at 1:25 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Tue, Mar 25, 2025 at 4:01 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > In the patch I also fixed a bug; I trusted XactReadOnly to see if the
> > local transaction is READ ONLY, but I noticed that that is not 100%
> > correct, because a transaction which started as READ WRITE can show as
> > READ ONLY later within subtransactions, so I modified the patch so
> > that postgres_fdw opens remote transactions in READ ONLY mode if the
> > local transaction has been declared READ ONLY at the top level.
>
> Nice catch. postgres_fdw replicates the transaction stack on foreign
> server. I think we need to replicate it along with the transaction
> properties. And also we need a test which tests readonly
> subtransaction behaviour.

Ok, will do.

Thanks for the comment!

Best regards,
Etsuro Fujita