On Thu, 2 Jun 2022 07:43:06 +0900
Michael Paquier <[email protected]> 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 <[email protected]>
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;
