Обсуждение: Prevent writes on large objects in read-only transactions

Hello,

Currently, lo_creat(e), lo_import, lo_unlink, lowrite, lo_put,
and lo_from_bytea are allowed even in read-only transactions.
By using them, pg_largeobject and pg_largeobject_metatable can
be modified in read-only transactions and the effect remains
after the transaction finished. Is it unacceptable behaviours, 
isn't it?

Also, when such transactions are used in recovery mode, it fails
but the messages output are not user friendly, like:

 postgres=# select lo_creat(42);
 ERROR:  cannot assign OIDs during recovery

 postgres=# select lo_create(42);
 ERROR:  cannot assign TransactionIds during recovery

 postgres=# select lo_unlink(16389);
 ERROR:  cannot acquire lock mode AccessExclusiveLock on database objects while recovery is in progress
 HINT:  Only RowExclusiveLock or less can be acquired on database objects during recovery.
 

So, I would like propose to explicitly prevent such writes operations
on large object in read-only transactions, like:

 postgres=# SELECT lo_create(42);
 ERROR:  cannot execute lo_create in a read-only transaction

The patch is attached.


Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

On Fri, 2022-05-27 at 15:30 +0900, Yugo NAGATA wrote:
> Currently, lo_creat(e), lo_import, lo_unlink, lowrite, lo_put,
> and lo_from_bytea are allowed even in read-only transactions.
> By using them, pg_largeobject and pg_largeobject_metatable can
> be modified in read-only transactions and the effect remains
> after the transaction finished. Is it unacceptable behaviours, 
> isn't it?

+1

Yours,
Laurenz Albe

On Fri, May 27, 2022 at 03:30:28PM +0900, Yugo NAGATA wrote:
> Currently, lo_creat(e), lo_import, lo_unlink, lowrite, lo_put,
> and lo_from_bytea are allowed even in read-only transactions.
> By using them, pg_largeobject and pg_largeobject_metatable can
> be modified in read-only transactions and the effect remains
> after the transaction finished. Is it unacceptable behaviours,
> isn't it?

Well, there is an actual risk to break applications that have worked
until now for a behavior that has existed for years with zero
complaints on the matter, so I would leave that alone.  Saying that, I
don't really disagree with improving the error messages a bit if we
are in recovery.
--
Michael

On Sat, 28 May 2022 18:00:54 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> On Fri, May 27, 2022 at 03:30:28PM +0900, Yugo NAGATA wrote:
> > Currently, lo_creat(e), lo_import, lo_unlink, lowrite, lo_put,
> > and lo_from_bytea are allowed even in read-only transactions.
> > By using them, pg_largeobject and pg_largeobject_metatable can
> > be modified in read-only transactions and the effect remains
> > after the transaction finished. Is it unacceptable behaviours, 
> > isn't it?
> 
> Well, there is an actual risk to break applications that have worked
> until now for a behavior that has existed for years with zero
> complaints on the matter, so I would leave that alone.  Saying that, I
> don't really disagree with improving the error messages a bit if we
> are in recovery.

Thank you for your comment. I am fine with leaving the behaviour in
read-only transactions as is if anyone don't complain and there are no
risks. 

As to the error messages during recovery, I think it is better to improve
it, because the current messages are emitted by elog() and it seems to imply
they are internal errors that we don't expected. I attached a updated patch
for it.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

On Mon, May 30, 2022 at 05:44:18PM +0900, Yugo NAGATA wrote:
> As to the error messages during recovery, I think it is better to improve
> it, because the current messages are emitted by elog() and it seems to imply
> they are internal errors that we don't expected. I attached a updated patch
> for it.

Yeah, elog() messages should never be user-facing as they refer to
internal errors, and any of those errors are rather deep in the tree
while being unexpected.

lo_write() is published in be-fsstubs.h, though we have no callers of
it in the backend for the core code.  Couldn't there be a point in
having the recovery protection there rather than in the upper SQL
routine be_lowrite()?  At the end, we would likely generate a failure
when attempting to insert the LO data in the catalogs through
inv_api.c, but I was wondering if we should make an extra effort in
improving the report also in this case if there is a direct caller of
this LO write routine.  The final picture may be better if we make
lo_write() a routine static to be-fsstubs.c but it is available for
ages, so I'd rather leave it declared as-is.

libpq fetches the OIDs of the large object functions and caches it for
PQfn() as far as I can see, so it is fine by me to have the
protections in be-fsstubs.c, letting inv_api.c deal with the internals
with the catalogs, ACL checks, etc.  Should we complain on lo_open()
with the write mode though?

The change for lo_truncate_internal() is a bit confusing for the 64b
version, as we would complain about lo_truncate() and not
lo_truncate64().

While on it, could we remove -DFSDB?
--
Michael

On Fri, May 27, 2022 at 02:02:24PM +0200, Laurenz Albe wrote:
> On Fri, 2022-05-27 at 15:30 +0900, Yugo NAGATA wrote:
>> Currently, lo_creat(e), lo_import, lo_unlink, lowrite, lo_put,
>> and lo_from_bytea are allowed even in read-only transactions.
>> By using them, pg_largeobject and pg_largeobject_metatable can
>> be modified in read-only transactions and the effect remains
>> after the transaction finished. Is it unacceptable behaviours,
>> isn't it?
>
> +1

And I have forgotten to add your name as a reviewer.  Sorry about
that!
--
Michael

On Sat, May 28, 2022 at 5:01 AM Michael Paquier <michael@paquier.xyz> wrote:
> Well, there is an actual risk to break applications that have worked
> until now for a behavior that has existed for years with zero
> complaints on the matter, so I would leave that alone.  Saying that, I
> don't really disagree with improving the error messages a bit if we
> are in recovery.

On the other hand, there's a good argument that the existing behavior
is simply incorrect.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, May 28, 2022 at 5:01 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Well, there is an actual risk to break applications that have worked
>> until now for a behavior that has existed for years with zero
>> complaints on the matter, so I would leave that alone.  Saying that, I
>> don't really disagree with improving the error messages a bit if we
>> are in recovery.

> On the other hand, there's a good argument that the existing behavior
> is simply incorrect.

Yeah.  Certainly we'd not want to back-patch this change, in case
anyone is relying on the current behavior ... but it's hard to argue
that it's not wrong.

What I'm wondering about is how far the principle of read-only-ness
ought to be expected to go.  Should a read-only transaction fail
to execute adminpack's pg_file_write(), for example?  Should it
refuse to execute random() on the grounds that that changes the
session's PRNG state?  The latter seems obviously silly, but
I'm not very sure about pg_file_write().  Maybe the restriction
should be "no changes to database state that's visible to other
sessions", which would leave outside-the-DB changes out of the
discussion.

            regards, tom lane

On Tue, May 31, 2022 at 3:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah.  Certainly we'd not want to back-patch this change, in case
> anyone is relying on the current behavior ... but it's hard to argue
> that it's not wrong.

Agreed.

> What I'm wondering about is how far the principle of read-only-ness
> ought to be expected to go.  Should a read-only transaction fail
> to execute adminpack's pg_file_write(), for example?  Should it
> refuse to execute random() on the grounds that that changes the
> session's PRNG state?  The latter seems obviously silly, but
> I'm not very sure about pg_file_write().  Maybe the restriction
> should be "no changes to database state that's visible to other
> sessions", which would leave outside-the-DB changes out of the
> discussion.

Yeah, I think that's a pretty good idea. It's really pretty hard to
imagine preventing outside-the-database writes in any kind of
principled way. Somebody can install a C function that does anything,
and we can do a pretty fair job preventing it from e.g. acquiring a
transaction ID or writing WAL by making changes in PostgreSQL itself,
but we can't prevent it from doing whatever it wants outside the
database. Nor is it even a very clear concept definitionally. I
wouldn't consider a function read-write solely on the basis that it
can cause data to be written to the PostgreSQL log file, for instance,
so it doesn't seem correct to suppose that a C function provided by an
extension is read-write just because it calls write(2) -- not that we
can detect that anyway, but even if we could.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, May 31, 2022 at 05:17:42PM -0400, Robert Haas wrote:
> On Tue, May 31, 2022 at 3:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I'm wondering about is how far the principle of read-only-ness
>> ought to be expected to go.  Should a read-only transaction fail
>> to execute adminpack's pg_file_write(), for example?  Should it
>> refuse to execute random() on the grounds that that changes the
>> session's PRNG state?  The latter seems obviously silly, but
>> I'm not very sure about pg_file_write().  Maybe the restriction
>> should be "no changes to database state that's visible to other
>> sessions", which would leave outside-the-DB changes out of the
>> discussion.
>
> Yeah, I think that's a pretty good idea. It's really pretty hard to
> imagine preventing outside-the-database writes in any kind of
> principled way. Somebody can install a C function that does anything,
> and we can do a pretty fair job preventing it from e.g. acquiring a
> transaction ID or writing WAL by making changes in PostgreSQL itself,
> but we can't prevent it from doing whatever it wants outside the
> database. Nor is it even a very clear concept definitionally. I
> wouldn't consider a function read-write solely on the basis that it
> can cause data to be written to the PostgreSQL log file, for instance,
> so it doesn't seem correct to suppose that a C function provided by an
> extension is read-write just because it calls write(2) -- not that we
> can detect that anyway, but even if we could.

Agreed.  There are a couple of arguments in authorizing
pg_file_write() in a read-only state or writes as long as it does not
affect WAL or the data.  For example, a change of configuration file
can be very useful at recovery if one wants to switch the
configuration (ALTER TABLE SET, etc.), so restricting functions that
perform writes outside the scope of WAL or the data does not make
sense to restrict.  Not to count updates in the control file, but
that's different.

Now the LO handling is quite old, and I am not sure if this is worth
changing as we have seen no actual complains about that with read-only
transactions, even if I agree on that it is inconsistent.  That could
cause more harm than the consistency benefit is worth :/
--
Michael

On Wed, Jun 1, 2022 at 1:29 AM Michael Paquier <michael@paquier.xyz> wrote:
> Now the LO handling is quite old, and I am not sure if this is worth
> changing as we have seen no actual complains about that with read-only
> transactions, even if I agree on that it is inconsistent.  That could
> cause more harm than the consistency benefit is worth :/

The message that started this thread is literally a complaint about
that exact thing.

We seem to do this fairly often on this list, honestly. Someone posts
a message saying "X is broken" and someone agrees and says it's a good
idea to fix it and then a third person responds and says "let's not
change it, no one has ever {noticed that,cared before,complained about
it}". I wonder whether the people who start such threads ever come to
the conclusion that the PostgreSQL community thinks that they are a
nobody and don't count.

As for the rest, I understand that changing the behavior creates an
incompatibility with previous releases, but I don't think we should be
worried about it. We create far larger incompatibilities in nearly
every release. There's probably very few people using large object
functions in read-only transactions compared to the number of people
using exclusive backup mode, or recovery.conf, or some
pg_stat_activity column that we decided to rename, or accessing
pg_xlog by name in some tool/script. I haven't really heard you
arguing vigorously against those changes, and it doesn't make sense to
me to hold this one, which to me seems to be vastly less likely to
break anything, to a higher standard.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jun 1, 2022 at 1:29 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Now the LO handling is quite old, and I am not sure if this is worth
>> changing as we have seen no actual complains about that with read-only
>> transactions, even if I agree on that it is inconsistent.  That could
>> cause more harm than the consistency benefit is worth :/

> The message that started this thread is literally a complaint about
> that exact thing.

Yeah.  I think this is more nearly "nobody had noticed" than "everybody
thinks this is okay".

> We seem to do this fairly often on this list, honestly. Someone posts
> a message saying "X is broken" and someone agrees and says it's a good
> idea to fix it and then a third person responds and says "let's not
> change it, no one has ever {noticed that,cared before,complained about
> it}".

It's always appropriate to consider backwards compatibility, and we
frequently don't back-patch a change because of worries about that.
However, if someone complains because we start rejecting this as of
v15 or v16, I don't think they have good grounds for that.  It's just
obviously wrong ... unless someone can come up with a plausible
definition of read-only-ness that excludes large objects.  I don't
say that that's impossible, but it sure seems like it'd be contorted
reasoning.  They're definitely inside-the-database entities.

            regards, tom lane

On Wed, Jun 01, 2022 at 10:15:17AM -0400, Tom Lane wrote:
> It's always appropriate to consider backwards compatibility, and we
> frequently don't back-patch a change because of worries about that.
> However, if someone complains because we start rejecting this as of
> v15 or v16, I don't think they have good grounds for that.  It's just
> obviously wrong ... unless someone can come up with a plausible
> definition of read-only-ness that excludes large objects.  I don't
> say that that's impossible, but it sure seems like it'd be contorted
> reasoning.  They're definitely inside-the-database entities.

FWIW, I find the removal of error paths to authorize new behaviors
easy to think about in terms of compatibility, because nobody is going
to complain about that as long as it works as intended.  The opposite,
aka enforcing an error in a code path is much harder to reason about.
Anyway, if I am outnumbered on this one that's fine by me :)

There are a couple of things in the original patch that may require to
be adjusted though:
1) Should we complain in lo_open() when using the write mode for a
read-only transaction?  My answer to that would be yes.
2) We still publish two non-fmgr-callable routines, lo_read() and
lo_write().  Perhaps we'd better make them static to be-fsstubs.c or
put the same restriction to the write routine as its SQL flavor?
--
Michael

On Thu, 2 Jun 2022 07:43:06 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> On Wed, Jun 01, 2022 at 10:15:17AM -0400, Tom Lane wrote:
> > It's always appropriate to consider backwards compatibility, and we
> > frequently don't back-patch a change because of worries about that.
> > However, if someone complains because we start rejecting this as of
> > v15 or v16, I don't think they have good grounds for that.  It's just
> > obviously wrong ... unless someone can come up with a plausible
> > definition of read-only-ness that excludes large objects.  I don't
> > say that that's impossible, but it sure seems like it'd be contorted
> > reasoning.  They're definitely inside-the-database entities.
> 
> FWIW, I find the removal of error paths to authorize new behaviors
> easy to think about in terms of compatibility, because nobody is going
> to complain about that as long as it works as intended.  The opposite,
> aka enforcing an error in a code path is much harder to reason about.
> Anyway, if I am outnumbered on this one that's fine by me :)

I attached the updated patch.

Per discussions above, I undo the change so that it prevents large
object writes in read-only transactions again.
 
> There are a couple of things in the original patch that may require to
> be adjusted though:
> 1) Should we complain in lo_open() when using the write mode for a
> read-only transaction?  My answer to that would be yes.

I fixed to raise the error in lo_open() when using the write mode.

> 2) We still publish two non-fmgr-callable routines, lo_read() and
> lo_write().  Pe4rhaps we'd better make them static to be-fsstubs.c or
> put the same restriction to the write routine as its SQL flavor?

I am not sure if we should use PreventCommandIfReadOnly in lo_write()
because there are other public functions that write to catalogs but there
are not the similar restrictions in such functions. I think it is caller's
responsibility to prevent to use such public functions in read-only context.

I also fixed to raise the error in each of lo_truncate and lo_truncate64
per Michael's comment in the other post.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

On Thu, Jun 16, 2022 at 03:42:06PM +0900, Yugo NAGATA wrote:
> I am not sure if we should use PreventCommandIfReadOnly in lo_write()
> because there are other public functions that write to catalogs but there
> are not the similar restrictions in such functions. I think it is caller's
> responsibility to prevent to use such public functions in read-only context.

I'd be really tempted to remove the plug on this one, actually.
However, that would also mean to break something just for the sake of
breaking it.  So perhaps you are right at the end in that it is better
to let this code be, without the new check.

> I also fixed to raise the error in each of lo_truncate and lo_truncate64
> per Michael's comment in the other post.

Thanks!  That counts for 10 SQL functions blocked with 10 tests.  So
you have all of them covered.

Looking at the docs of large objects, as of "Client Interfaces", we
mention that any action must take place in a transaction block.
Shouldn't we add a note that no write operations are allowed in a
read-only transaction?

+   if (mode & INV_WRITE)
+       PreventCommandIfReadOnly("lo_open() in write mode");
Nit.  This breaks translation.  I think that it could be switched to
"lo_open(INV_WRITE)" instead as the flag name is documented.

The patch is forgetting a refresh for largeobject_1.out.

---   INV_READ  = 0x20000
---   INV_WRITE = 0x40000
+--   INV_READ  = 0x40000
+--   INV_WRITE = 0x20000
Good catch!  This one is kind of independent, so I have fixed it
separately, in all the expected output files.
--
Michael

Hello Michael-san,

Thank you for reviewing the patch. I attached the updated patch.

On Thu, 16 Jun 2022 17:31:22 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> Looking at the docs of large objects, as of "Client Interfaces", we
> mention that any action must take place in a transaction block.
> Shouldn't we add a note that no write operations are allowed in a
> read-only transaction?

I added a description about read-only transaction to the doc.

> +   if (mode & INV_WRITE)
> +       PreventCommandIfReadOnly("lo_open() in write mode");
> Nit.  This breaks translation.  I think that it could be switched to
> "lo_open(INV_WRITE)" instead as the flag name is documented.

Changed it as you suggested.
 
> The patch is forgetting a refresh for largeobject_1.out.

I added changes for largeobject_1.out.

> ---   INV_READ  = 0x20000
> ---   INV_WRITE = 0x40000
> +--   INV_READ  = 0x40000
> +--   INV_WRITE = 0x20000
> Good catch!  This one is kind of independent, so I have fixed it
> separately, in all the expected output files.

Thanks!

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

On Wed, Jun 29, 2022 at 05:29:50PM +0900, Yugo NAGATA wrote:
> Thank you for reviewing the patch. I attached the updated patch.

Thanks for the new version.  I have looked at that again, and the set
of changes seem fine (including the change for the alternate output).
So, applied.
--
Michael

On Mon, 4 Jul 2022 15:51:32 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> On Wed, Jun 29, 2022 at 05:29:50PM +0900, Yugo NAGATA wrote:
> > Thank you for reviewing the patch. I attached the updated patch.
> 
> Thanks for the new version.  I have looked at that again, and the set
> of changes seem fine (including the change for the alternate output).
> So, applied.


Thanks!


-- 
Yugo NAGATA <nagata@sraoss.co.jp>