On Thu, 2 Jun 2022 07:43:06 +0900 Michael Paquier <mich...@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 <nag...@sraoss.co.jp>
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c index 5804532881..19c0a0c5de 100644 --- a/src/backend/libpq/be-fsstubs.c +++ b/src/backend/libpq/be-fsstubs.c @@ -93,6 +93,9 @@ be_lo_open(PG_FUNCTION_ARGS) elog(DEBUG4, "lo_open(%u,%d)", lobjId, mode); #endif + if (mode & INV_WRITE) + PreventCommandIfReadOnly("lo_open() in write mode"); + /* * Allocate a large object descriptor first. This will also create * 'fscxt' if this is the first LO opened in this transaction. @@ -179,6 +182,8 @@ lo_write(int fd, const char *buf, int len) int status; LargeObjectDesc *lobj; + PreventCommandIfReadOnly("lo_write"); + if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), @@ -245,6 +250,8 @@ be_lo_creat(PG_FUNCTION_ARGS) { Oid lobjId; + PreventCommandIfReadOnly("lo_creat()"); + lo_cleanup_needed = true; lobjId = inv_create(InvalidOid); @@ -256,6 +263,8 @@ be_lo_create(PG_FUNCTION_ARGS) { Oid lobjId = PG_GETARG_OID(0); + PreventCommandIfReadOnly("lo_create()"); + lo_cleanup_needed = true; lobjId = inv_create(lobjId); @@ -306,6 +315,8 @@ be_lo_unlink(PG_FUNCTION_ARGS) { Oid lobjId = PG_GETARG_OID(0); + PreventCommandIfReadOnly("lo_unlink()"); + /* * Must be owner of the large object. It would be cleaner to check this * in inv_drop(), but we want to throw the error before not after closing @@ -368,6 +379,8 @@ be_lowrite(PG_FUNCTION_ARGS) int bytestowrite; int totalwritten; + PreventCommandIfReadOnly("lowrite()"); + bytestowrite = VARSIZE_ANY_EXHDR(wbuf); totalwritten = lo_write(fd, VARDATA_ANY(wbuf), bytestowrite); PG_RETURN_INT32(totalwritten); @@ -413,6 +426,8 @@ lo_import_internal(text *filename, Oid lobjOid) LargeObjectDesc *lobj; Oid oid; + PreventCommandIfReadOnly("lo_import()"); + /* * open the file to be read in */ @@ -561,6 +576,8 @@ be_lo_truncate(PG_FUNCTION_ARGS) int32 fd = PG_GETARG_INT32(0); int32 len = PG_GETARG_INT32(1); + PreventCommandIfReadOnly("lo_truncate()"); + lo_truncate_internal(fd, len); PG_RETURN_INT32(0); } @@ -571,6 +588,8 @@ be_lo_truncate64(PG_FUNCTION_ARGS) int32 fd = PG_GETARG_INT32(0); int64 len = PG_GETARG_INT64(1); + PreventCommandIfReadOnly("lo_truncate64()"); + lo_truncate_internal(fd, len); PG_RETURN_INT32(0); } @@ -815,6 +834,8 @@ be_lo_from_bytea(PG_FUNCTION_ARGS) LargeObjectDesc *loDesc; int written PG_USED_FOR_ASSERTS_ONLY; + PreventCommandIfReadOnly("lo_from_bytea()"); + lo_cleanup_needed = true; loOid = inv_create(loOid); loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext); @@ -837,6 +858,8 @@ be_lo_put(PG_FUNCTION_ARGS) LargeObjectDesc *loDesc; int written PG_USED_FOR_ASSERTS_ONLY; + PreventCommandIfReadOnly("lo_put()"); + lo_cleanup_needed = true; loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext); diff --git a/src/test/regress/expected/largeobject.out b/src/test/regress/expected/largeobject.out index 8e32de04e6..86717e44df 100644 --- a/src/test/regress/expected/largeobject.out +++ b/src/test/regress/expected/largeobject.out @@ -50,8 +50,8 @@ INSERT INTO lotest_stash_values (loid) SELECT lo_creat(42); BEGIN; -- lo_open(lobjId oid, mode integer) returns integer -- The mode parameter to lo_open uses two constants: --- INV_READ = 0x20000 --- INV_WRITE = 0x40000 +-- INV_READ = 0x40000 +-- INV_WRITE = 0x20000 -- The return value is a file descriptor-like value which remains valid for the -- transaction. UPDATE lotest_stash_values SET fd = lo_open(loid, CAST(x'20000' | x'40000' AS integer)); @@ -506,6 +506,55 @@ SELECT lo_create(2121); (1 row) COMMENT ON LARGE OBJECT 2121 IS 'testing comments'; +-- Test writes on large objects in read-only transactions +START TRANSACTION READ ONLY; +-- INV_READ ... ok +SELECT lo_open(2121, x'40000'::int); + lo_open +--------- + 0 +(1 row) + +-- INV_WRITE ... error +SELECT lo_open(2121, x'20000'::int); +ERROR: cannot execute lo_open() in write mode in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lo_create(42); +ERROR: cannot execute lo_create() in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lo_creat(42); +ERROR: cannot execute lo_creat() in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lo_unlink(42); +ERROR: cannot execute lo_unlink() in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lowrite(42, 'x'); +ERROR: cannot execute lowrite() in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lo_import(:'filename'); +ERROR: cannot execute lo_import() in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lo_truncate(42, 0); +ERROR: cannot execute lo_truncate() in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lo_truncate64(42, 0); +ERROR: cannot execute lo_truncate64() in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lo_from_bytea(0, 'x'); +ERROR: cannot execute lo_from_bytea() in a read-only transaction +ROLLBACK; +START TRANSACTION READ ONLY; +SELECT lo_put(42, 0, 'x'); +ERROR: cannot execute lo_put() in a read-only transaction +ROLLBACK; -- Clean up DROP TABLE lotest_stash_values; DROP ROLE regress_lo_user; diff --git a/src/test/regress/sql/largeobject.sql b/src/test/regress/sql/largeobject.sql index fd3518889e..d3cb908fa9 100644 --- a/src/test/regress/sql/largeobject.sql +++ b/src/test/regress/sql/largeobject.sql @@ -34,8 +34,8 @@ BEGIN; -- lo_open(lobjId oid, mode integer) returns integer -- The mode parameter to lo_open uses two constants: --- INV_READ = 0x20000 --- INV_WRITE = 0x40000 +-- INV_READ = 0x40000 +-- INV_WRITE = 0x20000 -- The return value is a file descriptor-like value which remains valid for the -- transaction. UPDATE lotest_stash_values SET fd = lo_open(loid, CAST(x'20000' | x'40000' AS integer)); @@ -271,6 +271,50 @@ SELECT lo_create(2121); COMMENT ON LARGE OBJECT 2121 IS 'testing comments'; +-- Test writes on large objects in read-only transactions +START TRANSACTION READ ONLY; +-- INV_READ ... ok +SELECT lo_open(2121, x'40000'::int); +-- INV_WRITE ... error +SELECT lo_open(2121, x'20000'::int); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lo_create(42); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lo_creat(42); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lo_unlink(42); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lowrite(42, 'x'); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lo_import(:'filename'); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lo_truncate(42, 0); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lo_truncate64(42, 0); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lo_from_bytea(0, 'x'); +ROLLBACK; + +START TRANSACTION READ ONLY; +SELECT lo_put(42, 0, 'x'); +ROLLBACK; + -- Clean up DROP TABLE lotest_stash_values;