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 <mich...@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 <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/lobj.sgml b/doc/src/sgml/lobj.sgml
index b767ba1d05..2dbc95c4ad 100644
--- a/doc/src/sgml/lobj.sgml
+++ b/doc/src/sgml/lobj.sgml
@@ -114,7 +114,8 @@
All large object manipulation using these functions
<emphasis>must</emphasis> take place within an SQL transaction block,
since large object file descriptors are only valid for the duration of
- a transaction.
+ a transaction. Write operations, including <function>lo_open</function>
+ with <symbol>INV_WRITE</symbol> mode, are not allowed in a read-only transaction.
</para>
<para>
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 5804532881..043e47b93f 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(INV_WRITE)");
+
/*
* 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 60d7b991c1..31fba2ff9d 100644
--- a/src/test/regress/expected/largeobject.out
+++ b/src/test/regress/expected/largeobject.out
@@ -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(INV_WRITE) 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/expected/largeobject_1.out b/src/test/regress/expected/largeobject_1.out
index 30c8d3da80..7acd7f73e1 100644
--- a/src/test/regress/expected/largeobject_1.out
+++ b/src/test/regress/expected/largeobject_1.out
@@ -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(INV_WRITE) 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 2ef03cfdae..15e0dff7a3 100644
--- a/src/test/regress/sql/largeobject.sql
+++ b/src/test/regress/sql/largeobject.sql
@@ -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;
