Re: Prevent writes on large objects in read-only transactions

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>