On Mon, Mar 3, 2025 at 4:49 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Ashutosh Bapat <ashutosh.bapat....@gmail.com> writes: > > On Sun, Mar 2, 2025 at 5:14 PM Etsuro Fujita <etsuro.fuj...@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
Inherit-xact-properties-in-postgres-fdw-v2.patch
Description: Binary data
