Extend ALTER DEFAULT PRIVILEGES for large objects

[0]: https://www.postgresql.org/docs/devel/lo-intro.html

nagata@sraoss.co.jp

over 1 year ago

In reply to: Tom Lane (#2)

1 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Tue, 23 Apr 2024 23:47:38 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yugo NAGATA <nagata@sraoss.co.jp> writes:

Currently, ALTER DEFAULT PRIVILEGE doesn't support large objects,
so if we want to allow users other than the owner to use the large
object, we need to grant a privilege on it every time a large object
is created. One of our clients feels that this is annoying, so I would
like propose to extend ALTER DEFAULT PRIVILEGE to large objects.

I wonder how this plays with pg_dump, and in particular whether it
breaks the optimizations that a45c78e32 installed for large numbers
of large objects. The added test cases seem to go out of their way
to leave no trace behind that the pg_dump/pg_upgrade tests might
encounter.

Thank you for your comments.

The previous patch did not work with pg_dump since I forgot some fixes.
I attached a updated patch including fixes.

I believe a45c78e32 is about already-existing large objects and does
not directly related to default privileges, so will not be affected
by this patch.

I think you broke psql's \ddp, too. And some other places; grepping
for DEFACLOBJ_NAMESPACE finds other oversights.

Yes, I did. The attached patch include fixes for psql, too.

On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.

I believe this feature is beneficial to some users allows because
this enables to omit GRANT that was necessary every large object
creation. It seems to me that implementation/maintenance cost is not
so high compared to other objects (e.g. default privileges on schemas)
unless I am still missing something wrong.

Regards,
Yugo Nagata

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

Attachments:

v2-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-diff; name=v2-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload

From 0cfcdc2b297556248cfb64d67779d5fcb8dab227 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Fri, 8 Mar 2024 17:43:43 +0900
Subject: [PATCH v2] Extend ALTER DEFAULT PRIVILEGES for large objects

Original patch by Haruka Takatsuka, some fixes and tests by
Yugo Nagata.
---
 doc/src/sgml/catalogs.sgml                    |   3 +-
 .../sgml/ref/alter_default_privileges.sgml    |  15 ++-
 src/backend/catalog/aclchk.c                  |  21 ++++
 src/backend/catalog/objectaddress.c           |  18 ++-
 src/backend/catalog/pg_largeobject.c          |  18 ++-
 src/backend/parser/gram.y                     |   5 +-
 src/bin/pg_dump/dumputils.c                   |   3 +-
 src/bin/pg_dump/pg_dump.c                     |   3 +
 src/bin/psql/describe.c                       |   6 +-
 src/bin/psql/tab-complete.c                   |   2 +-
 src/include/catalog/pg_default_acl.h          |   1 +
 src/include/parser/kwlist.h                   |   1 +
 src/test/regress/expected/privileges.out      | 104 +++++++++++++++++-
 src/test/regress/sql/privileges.sql           |  47 ++++++++
 14 files changed, 235 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..b8cc822aeb 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -3330,7 +3330,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
        <literal>S</literal> = sequence,
        <literal>f</literal> = function,
        <literal>T</literal> = type,
-       <literal>n</literal> = schema
+       <literal>n</literal> = schema,
+       <literal>L</literal> = large object
       </para></entry>
      </row>
 
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 1de4c5c1b4..3b358b7a88 100644
--- a/doc/src/sgml/ref/alter_default_privileges.sgml
+++ b/doc/src/sgml/ref/alter_default_privileges.sgml
@@ -50,6 +50,11 @@ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
     ON SCHEMAS
     TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
 
+GRANT { { SELECT | UPDATE }
+    [, ...] | ALL [ PRIVILEGES ] }
+    ON LARGE OBJECTS
+    TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
 REVOKE [ GRANT OPTION FOR ]
     { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
     [, ...] | ALL [ PRIVILEGES ] }
@@ -81,6 +86,13 @@ REVOKE [ GRANT OPTION FOR ]
     ON SCHEMAS
     FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
     [ CASCADE | RESTRICT ]
+
+REVOKE [ GRANT OPTION FOR ]
+    { { SELECT | UPDATE }
+    [, ...] | ALL [ PRIVILEGES ] }
+    ON LARGE OBJECTS
+    FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
+    [ CASCADE | RESTRICT ]
 </synopsis>
  </refsynopsisdiv>
 
@@ -159,7 +171,8 @@ REVOKE [ GRANT OPTION FOR ]
       If <literal>IN SCHEMA</literal> is omitted, the global default privileges
       are altered.
       <literal>IN SCHEMA</literal> is not allowed when setting privileges
-      for schemas, since schemas can't be nested.
+      for schemas and large objects, since schemas can't be nested and
+      large objects don't belong to a schema.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..41baf81a1d 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -1077,6 +1077,10 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
 			all_privileges = ACL_ALL_RIGHTS_SCHEMA;
 			errormsg = gettext_noop("invalid privilege type %s for schema");
 			break;
+		case OBJECT_LARGEOBJECT:
+			all_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+			errormsg = gettext_noop("invalid privilege type %s for large object");
+			break;
 		default:
 			elog(ERROR, "unrecognized GrantStmt.objtype: %d",
 				 (int) action->objtype);
@@ -1268,6 +1272,16 @@ SetDefaultACL(InternalDefaultACL *iacls)
 				this_privileges = ACL_ALL_RIGHTS_SCHEMA;
 			break;
 
+		case OBJECT_LARGEOBJECT:
+			if (OidIsValid(iacls->nspid))
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+						 errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS")));
+			objtype = DEFACLOBJ_LARGEOBJECT;
+			if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+				this_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+			break;
+
 		default:
 			elog(ERROR, "unrecognized object type: %d",
 				 (int) iacls->objtype);
@@ -1499,6 +1513,9 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
 			case DEFACLOBJ_NAMESPACE:
 				iacls.objtype = OBJECT_SCHEMA;
 				break;
+			case DEFACLOBJ_LARGEOBJECT:
+				iacls.objtype = OBJECT_LARGEOBJECT;
+				break;
 			default:
 				/* Shouldn't get here */
 				elog(ERROR, "unexpected default ACL type: %d",
@@ -4324,6 +4341,10 @@ get_user_default_acl(ObjectType objtype, Oid ownerId, Oid nsp_oid)
 			defaclobjtype = DEFACLOBJ_NAMESPACE;
 			break;
 
+		case OBJECT_LARGEOBJECT:
+			defaclobjtype = DEFACLOBJ_LARGEOBJECT;
+			break;
+
 		default:
 			return NULL;
 	}
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 7b536ac6fd..5b330967a6 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2000,16 +2000,20 @@ get_object_address_defacl(List *object, bool missing_ok)
 		case DEFACLOBJ_NAMESPACE:
 			objtype_str = "schemas";
 			break;
+		case DEFACLOBJ_LARGEOBJECT:
+			objtype_str = "large objects";
+			break;
 		default:
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					 errmsg("unrecognized default ACL object type \"%c\"", objtype),
-					 errhint("Valid object types are \"%c\", \"%c\", \"%c\", \"%c\", \"%c\".",
+					 errhint("Valid object types are \"%c\", \"%c\", \"%c\", \"%c\", \"%c\", \"%c\".",
 							 DEFACLOBJ_RELATION,
 							 DEFACLOBJ_SEQUENCE,
 							 DEFACLOBJ_FUNCTION,
 							 DEFACLOBJ_TYPE,
-							 DEFACLOBJ_NAMESPACE)));
+							 DEFACLOBJ_NAMESPACE,
+							 DEFACLOBJ_LARGEOBJECT)));
 	}
 
 	/*
@@ -3798,6 +3802,12 @@ getObjectDescription(const ObjectAddress *object, bool missing_ok)
 										 _("default privileges on new schemas belonging to role %s"),
 										 rolename);
 						break;
+					case DEFACLOBJ_LARGEOBJECT:
+						Assert(!nspname);
+						appendStringInfo(&buffer,
+										 _("default privileges on new large objects belonging to role %s"),
+										 rolename);
+						break;
 					default:
 						/* shouldn't get here */
 						if (nspname)
@@ -5651,6 +5661,10 @@ getObjectIdentityParts(const ObjectAddress *object,
 						appendStringInfoString(&buffer,
 											   " on schemas");
 						break;
+					case DEFACLOBJ_LARGEOBJECT:
+						appendStringInfoString(&buffer,
+											   " on large objects");
+						break;
 				}
 
 				if (objname)
diff --git a/src/backend/catalog/pg_largeobject.c b/src/backend/catalog/pg_largeobject.c
index e235f7c5e6..578589b457 100644
--- a/src/backend/catalog/pg_largeobject.c
+++ b/src/backend/catalog/pg_largeobject.c
@@ -22,6 +22,7 @@
 #include "catalog/pg_largeobject.h"
 #include "catalog/pg_largeobject_metadata.h"
 #include "miscadmin.h"
+#include "utils/acl.h"
 #include "utils/fmgroids.h"
 #include "utils/rel.h"
 
@@ -41,6 +42,8 @@ LargeObjectCreate(Oid loid)
 	Oid			loid_new;
 	Datum		values[Natts_pg_largeobject_metadata];
 	bool		nulls[Natts_pg_largeobject_metadata];
+	Oid			ownerId;
+	Acl			*lomacl;
 
 	pg_lo_meta = table_open(LargeObjectMetadataRelationId,
 							RowExclusiveLock);
@@ -57,11 +60,18 @@ LargeObjectCreate(Oid loid)
 		loid_new = GetNewOidWithIndex(pg_lo_meta,
 									  LargeObjectMetadataOidIndexId,
 									  Anum_pg_largeobject_metadata_oid);
+	ownerId = GetUserId();
+	lomacl = get_user_default_acl(OBJECT_LARGEOBJECT, ownerId, InvalidOid);
 
 	values[Anum_pg_largeobject_metadata_oid - 1] = ObjectIdGetDatum(loid_new);
 	values[Anum_pg_largeobject_metadata_lomowner - 1]
-		= ObjectIdGetDatum(GetUserId());
-	nulls[Anum_pg_largeobject_metadata_lomacl - 1] = true;
+		= ObjectIdGetDatum(ownerId);
+
+	if (lomacl != NULL)
+		values[Anum_pg_largeobject_metadata_lomacl - 1]
+			= PointerGetDatum(lomacl);
+	else
+		nulls[Anum_pg_largeobject_metadata_lomacl - 1] = true;
 
 	ntup = heap_form_tuple(RelationGetDescr(pg_lo_meta),
 						   values, nulls);
@@ -72,6 +82,10 @@ LargeObjectCreate(Oid loid)
 
 	table_close(pg_lo_meta, RowExclusiveLock);
 
+	/* dependencies on roles mentioned in default ACL */
+	recordDependencyOnNewAcl(LargeObjectRelationId, loid_new, 0,
+							 ownerId, lomacl);
+
 	return loid_new;
 }
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e8b619926e..35535ab390 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -760,7 +760,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
 	NULLS_P NUMERIC
 
-	OBJECT_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
+	OBJECT_P OBJECTS_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
 	ORDER ORDINALITY OTHERS OUT_P OUTER_P
 	OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER
 
@@ -8170,6 +8170,7 @@ defacl_privilege_target:
 			| SEQUENCES		{ $$ = OBJECT_SEQUENCE; }
 			| TYPES_P		{ $$ = OBJECT_TYPE; }
 			| SCHEMAS		{ $$ = OBJECT_SCHEMA; }
+			| LARGE_P OBJECTS_P	{ $$ = OBJECT_LARGEOBJECT; }
 		;
 
 
@@ -17788,6 +17789,7 @@ unreserved_keyword:
 			| NOWAIT
 			| NULLS_P
 			| OBJECT_P
+			| OBJECTS_P
 			| OF
 			| OFF
 			| OIDS
@@ -18411,6 +18413,7 @@ bare_label_keyword:
 			| NULLS_P
 			| NUMERIC
 			| OBJECT_P
+			| OBJECTS_P
 			| OF
 			| OFF
 			| OIDS
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 5649859aa1..0fad29cbaf 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -506,7 +506,8 @@ do { \
 		CONVERT_PRIV('s', "SET");
 		CONVERT_PRIV('A', "ALTER SYSTEM");
 	}
-	else if (strcmp(type, "LARGE OBJECT") == 0)
+	else if (strcmp(type, "LARGE OBJECT") == 0 ||
+			 strcmp(type, "LARGE OBJECTS") == 0)
 	{
 		CONVERT_PRIV('r', "SELECT");
 		CONVERT_PRIV('w', "UPDATE");
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index b8acdd7355..298cbb3d56 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15224,6 +15224,9 @@ dumpDefaultACL(Archive *fout, const DefaultACLInfo *daclinfo)
 		case DEFACLOBJ_NAMESPACE:
 			type = "SCHEMAS";
 			break;
+		case DEFACLOBJ_LARGEOBJECT:
+			type = "LARGE OBJECTS";
+			break;
 		default:
 			/* shouldn't get here */
 			pg_fatal("unrecognized object type in default privileges: %d",
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 4a9ee4a54d..12d002b709 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1184,7 +1184,9 @@ listDefaultACLs(const char *pattern)
 	printfPQExpBuffer(&buf,
 					  "SELECT pg_catalog.pg_get_userbyid(d.defaclrole) AS \"%s\",\n"
 					  "  n.nspname AS \"%s\",\n"
-					  "  CASE d.defaclobjtype WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' END AS \"%s\",\n"
+					  "  CASE d.defaclobjtype "
+					  "    WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s'"
+					  "    WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' END AS \"%s\",\n"
 					  "  ",
 					  gettext_noop("Owner"),
 					  gettext_noop("Schema"),
@@ -1198,6 +1200,8 @@ listDefaultACLs(const char *pattern)
 					  gettext_noop("type"),
 					  DEFACLOBJ_NAMESPACE,
 					  gettext_noop("schema"),
+					  DEFACLOBJ_LARGEOBJECT,
+					  gettext_noop("large object"),
 					  gettext_noop("Type"));
 
 	printACLColumn(&buf, "d.defaclacl");
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6fee3160f0..03889fc52f 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4035,7 +4035,7 @@ psql_completion(const char *text, int start, int end)
 		 * objects supported.
 		 */
 		if (HeadMatches("ALTER", "DEFAULT", "PRIVILEGES"))
-			COMPLETE_WITH("TABLES", "SEQUENCES", "FUNCTIONS", "PROCEDURES", "ROUTINES", "TYPES", "SCHEMAS");
+			COMPLETE_WITH("TABLES", "SEQUENCES", "FUNCTIONS", "PROCEDURES", "ROUTINES", "TYPES", "SCHEMAS", "LARGE OBJECTS");
 		else
 			COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_grantables,
 											"ALL FUNCTIONS IN SCHEMA",
diff --git a/src/include/catalog/pg_default_acl.h b/src/include/catalog/pg_default_acl.h
index d272cdf08b..f9f002fa45 100644
--- a/src/include/catalog/pg_default_acl.h
+++ b/src/include/catalog/pg_default_acl.h
@@ -68,6 +68,7 @@ MAKE_SYSCACHE(DEFACLROLENSPOBJ, pg_default_acl_role_nsp_obj_index, 8);
 #define DEFACLOBJ_FUNCTION		'f' /* function */
 #define DEFACLOBJ_TYPE			'T' /* type */
 #define DEFACLOBJ_NAMESPACE		'n' /* namespace */
+#define DEFACLOBJ_LARGEOBJECT	'L' /* large object */
 
 #endif							/* EXPOSE_TO_CLIENT_CODE */
 
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index f9a4afd472..6299c29389 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -307,6 +307,7 @@ PG_KEYWORD("nullif", NULLIF, COL_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("nulls", NULLS_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("numeric", NUMERIC, COL_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("object", OBJECT_P, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("objects", OBJECTS_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("of", OF, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("off", OFF, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("offset", OFFSET, RESERVED_KEYWORD, AS_LABEL)
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index eb4b762ea1..29f0775373 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2357,11 +2357,110 @@ SELECT has_schema_privilege('regress_priv_user2', 'testns4', 'CREATE'); -- yes
 
 ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 COMMIT;
+--
+-- Test for default privileges on large objects. This is done in a
+-- separate, rollbacked, transaction to avoid any trouble with other
+-- regression sessions.
+--
+\c -
+SET SESSION AUTHORIZATION regress_priv_user1;
+SELECT lo_create(1007);
+ lo_create 
+-----------
+      1007
+(1 row)
+
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT loread(lo_open(1007, x'40000'::int), 32); -- to be denied
+ERROR:  permission denied for large object 1007
+SELECT lowrite(lo_open(1007, x'20000'::int), 'abcd'); -- to be denied
+ERROR:  permission denied for large object 1007
+BEGIN;
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON LARGE OBJECTS TO public;
+SELECT lo_unlink(1007);
+ lo_unlink 
+-----------
+         1
+(1 row)
+
+SELECT lo_create(1007);
+ lo_create 
+-----------
+      1007
+(1 row)
+
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT loread(lo_open(1007, x'40000'::int), 32); -- ok
+ loread 
+--------
+ \x
+(1 row)
+
+SELECT lowrite(lo_open(1007, x'20000'::int), 'abcd'); -- to be denied
+ERROR:  permission denied for large object 1007
+ROLLBACK;
+BEGIN;
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER DEFAULT PRIVILEGES GRANT SELECT, UPDATE ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_unlink(1007);
+ lo_unlink 
+-----------
+         1
+(1 row)
+
+SELECT lo_create(1007);
+ lo_create 
+-----------
+      1007
+(1 row)
+
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT loread(lo_open(1007, x'40000'::int), 32); -- ok
+ loread 
+--------
+ \x
+(1 row)
+
+SELECT lowrite(lo_open(1007, x'20000'::int), 'abcd'); -- ok
+ lowrite 
+---------
+       4
+(1 row)
+
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER DEFAULT PRIVILEGES REVOKE UPDATE ON LARGE OBJECTS FROM regress_priv_user2;
+SELECT lo_unlink(1007);
+ lo_unlink 
+-----------
+         1
+(1 row)
+
+SELECT lo_create(1007);
+ lo_create 
+-----------
+      1007
+(1 row)
+
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT loread(lo_open(1007, x'40000'::int), 32); -- ok
+ loread 
+--------
+ \x
+(1 row)
+
+SELECT lowrite(lo_open(1007, x'20000'::int), 'abcd');	-- to be denied
+ERROR:  permission denied for large object 1007
+ROLLBACK;
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON LARGE OBJECTS TO public; -- error
+ERROR:  cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS
+\c -
 -- Test for DROP OWNED BY with shared dependencies.  This is done in a
 -- separate, rollbacked, transaction to avoid any trouble with other
 -- regression sessions.
 BEGIN;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON FUNCTIONS TO regress_priv_user2;
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SCHEMAS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SEQUENCES TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO regress_priv_user2;
@@ -2372,7 +2471,7 @@ SELECT count(*) FROM pg_shdepend
 	classid = 'pg_default_acl'::regclass;
  count 
 -------
-     5
+     6
 (1 row)
 
 DROP OWNED BY regress_priv_user2, regress_priv_user2;
@@ -2708,7 +2807,8 @@ SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >= 1000 AND oid < 3
          1
          1
          1
-(5 rows)
+         1
+(6 rows)
 
 DROP GROUP regress_priv_group1;
 DROP GROUP regress_priv_group2;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index eeb4c00292..e8db20573c 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1487,11 +1487,58 @@ ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 
 COMMIT;
 
+--
+-- Test for default privileges on large objects. This is done in a
+-- separate, rollbacked, transaction to avoid any trouble with other
+-- regression sessions.
+--
+\c -
+SET SESSION AUTHORIZATION regress_priv_user1;
+SELECT lo_create(1007);
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT loread(lo_open(1007, x'40000'::int), 32); -- to be denied
+SELECT lowrite(lo_open(1007, x'20000'::int), 'abcd'); -- to be denied
+
+BEGIN;
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON LARGE OBJECTS TO public;
+SELECT lo_unlink(1007);
+SELECT lo_create(1007);
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT loread(lo_open(1007, x'40000'::int), 32); -- ok
+SELECT lowrite(lo_open(1007, x'20000'::int), 'abcd'); -- to be denied
+ROLLBACK;
+
+BEGIN;
+
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER DEFAULT PRIVILEGES GRANT SELECT, UPDATE ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_unlink(1007);
+SELECT lo_create(1007);
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT loread(lo_open(1007, x'40000'::int), 32); -- ok
+SELECT lowrite(lo_open(1007, x'20000'::int), 'abcd'); -- ok
+
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER DEFAULT PRIVILEGES REVOKE UPDATE ON LARGE OBJECTS FROM regress_priv_user2;
+SELECT lo_unlink(1007);
+SELECT lo_create(1007);
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT loread(lo_open(1007, x'40000'::int), 32); -- ok
+SELECT lowrite(lo_open(1007, x'20000'::int), 'abcd');	-- to be denied
+
+ROLLBACK;
+
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON LARGE OBJECTS TO public; -- error
+
+\c -
+
 -- Test for DROP OWNED BY with shared dependencies.  This is done in a
 -- separate, rollbacked, transaction to avoid any trouble with other
 -- regression sessions.
 BEGIN;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON FUNCTIONS TO regress_priv_user2;
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SCHEMAS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SEQUENCES TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO regress_priv_user2;
-- 
2.25.1

Nathan Bossart

nathandbossart@gmail.com

over 1 year ago

In reply to: Tom Lane (#2)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:

On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.

I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0]https://www.postgresql.org/docs/devel/lo-intro.html that might be worth examining.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

[0]: /messages/by-id/flat/CAN-LCVMq2X=fhx7KLxfeDyb3P+BXuCkHC0g=9GF+JD4izfVa0Q@mail.gmail.com
[1]: /messages/by-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22=5xVBg7S4vr5rQ@mail.gmail.com
[2]: /messages/by-id/CAN-LCVOgMrda9hOdzGkCMdwY6dH0JQa13QvPsqUwY57TEn6jww@mail.gmail.com

nagata@sraoss.co.jp

over 1 year ago

In reply to: Nathan Bossart (#4)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Wed, 24 Apr 2024 16:08:39 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:

On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:

On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.

I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0] that might be worth examining.

[0] https://www.postgresql.org/docs/devel/lo-intro.html

If we could replace large objects with BYTEA in any use cases, large objects
would be completely obsolete. However, currently some users use large objects
in fact, so improvement in this feature seems beneficial for them.

Apart from that, extending TOAST to support more than 1GB data and
stream-style access seems a good challenge. I don't know if there was a
proposal for this in past. This is just a thought, for this purpose, we
will need a new type of varlena that can contains large size information,
and a new toast table schema that can store offset information or some way
to convert a offset to chunk_seq.

Regards,
Yugo Nagata

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

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

Matthias van de Meent

boekewurm+postgres@gmail.com

over 1 year ago

In reply to: Yugo NAGATA (#5)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Fri, 26 Apr 2024 at 10:54, Yugo NAGATA <nagata@sraoss.co.jp> wrote:

On Wed, 24 Apr 2024 16:08:39 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:

On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:

On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.

I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0] that might be worth examining.

[0] https://www.postgresql.org/docs/devel/lo-intro.html

If we could replace large objects with BYTEA in any use cases, large objects
would be completely obsolete. However, currently some users use large objects
in fact, so improvement in this feature seems beneficial for them.

Apart from that, extending TOAST to support more than 1GB data and
stream-style access seems a good challenge. I don't know if there was a
proposal for this in past. This is just a thought, for this purpose, we
will need a new type of varlena that can contains large size information,
and a new toast table schema that can store offset information or some way
to convert a offset to chunk_seq.

If you're interested in this, you may want to check out [0]/messages/by-id/flat/CAN-LCVMq2X=fhx7KLxfeDyb3P+BXuCkHC0g=9GF+JD4izfVa0Q@mail.gmail.com and [1]/messages/by-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22=5xVBg7S4vr5rQ@mail.gmail.com as
threads on the topic of improving TOAST handling of large values ([1]/messages/by-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22=5xVBg7S4vr5rQ@mail.gmail.com
being a thread where the limitations of our current external TOAST
pointer became clear once more), and maybe talk with Aleksander
Alekseev and Nikita Malakhov. They've been working closely with
systems that involve toast pointers and their limitations.

The most recent update on the work of Nikita (reworking TOAST
handling) [2]/messages/by-id/CAN-LCVOgMrda9hOdzGkCMdwY6dH0JQa13QvPsqUwY57TEn6jww@mail.gmail.com is that he got started adapting their externally
pluggable toast into type-internal methods only, though I've not yet
noticed any updated patches appear on the list.

As for other issues with creating larger TOAST values:
TOAST has a value limit of ~1GB, which means a single large value (or
two, for that matter) won't break anything in the wire protocol, as
DataRow messages have a message size field of uint32 [^3]. However, if
we're going to allow even larger values to be stored in table's
attributes, we'll have to figure out how we're going to transfer those
larger values to (and from) clients. For large objects, this is much
less of an issue because the IO operations are already chunked by
design, but this may not work well for types that you want to use in
your table's columns.

Kind regards,

Matthias van de Meent

[^3] Most, if not all PostgreSQL wire protocol messages have this
uint32 message size field, but the DataRow one is relevant here as
it's the one way users get their data out of the database.

nagata@sraoss.co.jp

over 1 year ago

In reply to: Matthias van de Meent (#6)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Fri, 26 Apr 2024 12:23:45 +0200
Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:

On Fri, 26 Apr 2024 at 10:54, Yugo NAGATA <nagata@sraoss.co.jp> wrote:

On Wed, 24 Apr 2024 16:08:39 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:

On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:

On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.

I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0] that might be worth examining.

[0] https://www.postgresql.org/docs/devel/lo-intro.html

If we could replace large objects with BYTEA in any use cases, large objects
would be completely obsolete. However, currently some users use large objects
in fact, so improvement in this feature seems beneficial for them.

Apart from that, extending TOAST to support more than 1GB data and
stream-style access seems a good challenge. I don't know if there was a
proposal for this in past. This is just a thought, for this purpose, we
will need a new type of varlena that can contains large size information,
and a new toast table schema that can store offset information or some way
to convert a offset to chunk_seq.

If you're interested in this, you may want to check out [0] and [1] as
threads on the topic of improving TOAST handling of large values ([1]
being a thread where the limitations of our current external TOAST
pointer became clear once more), and maybe talk with Aleksander
Alekseev and Nikita Malakhov. They've been working closely with
systems that involve toast pointers and their limitations.

The most recent update on the work of Nikita (reworking TOAST
handling) [2] is that he got started adapting their externally
pluggable toast into type-internal methods only, though I've not yet
noticed any updated patches appear on the list.

Thank you for your information. I'll check the threads you mentioned.

As for other issues with creating larger TOAST values:
TOAST has a value limit of ~1GB, which means a single large value (or
two, for that matter) won't break anything in the wire protocol, as
DataRow messages have a message size field of uint32 [^3]. However, if
we're going to allow even larger values to be stored in table's
attributes, we'll have to figure out how we're going to transfer those
larger values to (and from) clients. For large objects, this is much
less of an issue because the IO operations are already chunked by
design, but this may not work well for types that you want to use in
your table's columns.

I overlooked this issue. I faced the similar issue when I tried to
pg_dump large text values, although the error was raised from
enlargeStringInfo() in that case....

Regards,
Yugo Nagata

Kind regards,

Matthias van de Meent

[0] /messages/by-id/flat/CAN-LCVMq2X=fhx7KLxfeDyb3P+BXuCkHC0g=9GF+JD4izfVa0Q@mail.gmail.com
[1] /messages/by-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22=5xVBg7S4vr5rQ@mail.gmail.com
[2] /messages/by-id/CAN-LCVOgMrda9hOdzGkCMdwY6dH0JQa13QvPsqUwY57TEn6jww@mail.gmail.com

[^3] Most, if not all PostgreSQL wire protocol messages have this
uint32 message size field, but the DataRow one is relevant here as
it's the one way users get their data out of the database.

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

nagata@sraoss.co.jp

over 1 year ago

In reply to: Yugo NAGATA (#5)

1 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Fri, 26 Apr 2024 17:54:06 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:

On Wed, 24 Apr 2024 16:08:39 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:

On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote:

On the whole I find this proposed feature pretty unexciting
and dubiously worthy of the implementation/maintenance effort.

I don't have any particularly strong feelings on $SUBJECT, but I'll admit
I'd be much more interested in resolving any remaining reasons folks are
using large objects over TOAST. I see a couple of reasons listed in the
docs [0] that might be worth examining.

[0] https://www.postgresql.org/docs/devel/lo-intro.html

If we could replace large objects with BYTEA in any use cases, large objects
would be completely obsolete. However, currently some users use large objects
in fact, so improvement in this feature seems beneficial for them.

I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.

Regards,
Yugo Nagata

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

Attachments:

v3-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-diff; name=v3-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload

From a27680b9f3031b0995fe20dd2d166e07a17ffeca Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Fri, 8 Mar 2024 17:43:43 +0900
Subject: [PATCH v3] Extend ALTER DEFAULT PRIVILEGES for large objects

Previously, ALTER DEFAULT PRIVILEGE didn't support large objects,
so if we want to allow users other than the owner to use the large
object, we need to grant a privilege on it every time a large object
is created.

Original patch by Haruka Takatsuka, some fixes and tests by
Yugo Nagata.
---
 doc/src/sgml/catalogs.sgml                    |  3 +-
 .../sgml/ref/alter_default_privileges.sgml    | 15 ++-
 src/backend/catalog/aclchk.c                  | 21 +++++
 src/backend/catalog/objectaddress.c           | 18 +++-
 src/backend/catalog/pg_largeobject.c          | 18 +++-
 src/backend/parser/gram.y                     |  5 +-
 src/bin/pg_dump/dumputils.c                   |  3 +-
 src/bin/pg_dump/pg_dump.c                     |  3 +
 src/bin/psql/describe.c                       |  6 +-
 src/bin/psql/tab-complete.c                   |  2 +-
 src/include/catalog/pg_default_acl.h          |  1 +
 src/include/parser/kwlist.h                   |  1 +
 src/test/regress/expected/privileges.out      | 94 ++++++++++++++++++-
 src/test/regress/sql/privileges.sql           | 36 +++++++
 14 files changed, 215 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b654fae1b2..c8cf56c666 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -3322,7 +3322,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
        <literal>S</literal> = sequence,
        <literal>f</literal> = function,
        <literal>T</literal> = type,
-       <literal>n</literal> = schema
+       <literal>n</literal> = schema,
+       <literal>L</literal> = large object
       </para></entry>
      </row>
 
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 89aacec4fa..3ab695892d 100644
--- a/doc/src/sgml/ref/alter_default_privileges.sgml
+++ b/doc/src/sgml/ref/alter_default_privileges.sgml
@@ -51,6 +51,11 @@ GRANT { { USAGE | CREATE }
     ON SCHEMAS
     TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
 
+GRANT { { SELECT | UPDATE }
+    [, ...] | ALL [ PRIVILEGES ] }
+    ON LARGE OBJECTS
+    TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
 REVOKE [ GRANT OPTION FOR ]
     { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
     [, ...] | ALL [ PRIVILEGES ] }
@@ -83,6 +88,13 @@ REVOKE [ GRANT OPTION FOR ]
     ON SCHEMAS
     FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
     [ CASCADE | RESTRICT ]
+
+REVOKE [ GRANT OPTION FOR ]
+    { { SELECT | UPDATE }
+    [, ...] | ALL [ PRIVILEGES ] }
+    ON LARGE OBJECTS
+    FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
+    [ CASCADE | RESTRICT ]
 </synopsis>
  </refsynopsisdiv>
 
@@ -161,7 +173,8 @@ REVOKE [ GRANT OPTION FOR ]
       If <literal>IN SCHEMA</literal> is omitted, the global default privileges
       are altered.
       <literal>IN SCHEMA</literal> is not allowed when setting privileges
-      for schemas, since schemas can't be nested.
+      for schemas and large objects, since schemas can't be nested and
+      large objects don't belong to a schema.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index d2abc48fd8..6463ae921a 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -1077,6 +1077,10 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
 			all_privileges = ACL_ALL_RIGHTS_SCHEMA;
 			errormsg = gettext_noop("invalid privilege type %s for schema");
 			break;
+		case OBJECT_LARGEOBJECT:
+			all_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+			errormsg = gettext_noop("invalid privilege type %s for large object");
+			break;
 		default:
 			elog(ERROR, "unrecognized GrantStmt.objtype: %d",
 				 (int) action->objtype);
@@ -1268,6 +1272,16 @@ SetDefaultACL(InternalDefaultACL *iacls)
 				this_privileges = ACL_ALL_RIGHTS_SCHEMA;
 			break;
 
+		case OBJECT_LARGEOBJECT:
+			if (OidIsValid(iacls->nspid))
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+						 errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS")));
+			objtype = DEFACLOBJ_LARGEOBJECT;
+			if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+				this_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+			break;
+
 		default:
 			elog(ERROR, "unrecognized object type: %d",
 				 (int) iacls->objtype);
@@ -1511,6 +1525,9 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
 			case DEFACLOBJ_NAMESPACE:
 				iacls.objtype = OBJECT_SCHEMA;
 				break;
+			case DEFACLOBJ_LARGEOBJECT:
+				iacls.objtype = OBJECT_LARGEOBJECT;
+				break;
 			default:
 				/* Shouldn't get here */
 				elog(ERROR, "unexpected default ACL type: %d",
@@ -4334,6 +4351,10 @@ get_user_default_acl(ObjectType objtype, Oid ownerId, Oid nsp_oid)
 			defaclobjtype = DEFACLOBJ_NAMESPACE;
 			break;
 
+		case OBJECT_LARGEOBJECT:
+			defaclobjtype = DEFACLOBJ_LARGEOBJECT;
+			break;
+
 		default:
 			return NULL;
 	}
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 85a7b7e641..94f38c2333 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2000,16 +2000,20 @@ get_object_address_defacl(List *object, bool missing_ok)
 		case DEFACLOBJ_NAMESPACE:
 			objtype_str = "schemas";
 			break;
+		case DEFACLOBJ_LARGEOBJECT:
+			objtype_str = "large objects";
+			break;
 		default:
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					 errmsg("unrecognized default ACL object type \"%c\"", objtype),
-					 errhint("Valid object types are \"%c\", \"%c\", \"%c\", \"%c\", \"%c\".",
+					 errhint("Valid object types are \"%c\", \"%c\", \"%c\", \"%c\", \"%c\", \"%c\".",
 							 DEFACLOBJ_RELATION,
 							 DEFACLOBJ_SEQUENCE,
 							 DEFACLOBJ_FUNCTION,
 							 DEFACLOBJ_TYPE,
-							 DEFACLOBJ_NAMESPACE)));
+							 DEFACLOBJ_NAMESPACE,
+							 DEFACLOBJ_LARGEOBJECT)));
 	}
 
 	/*
@@ -3798,6 +3802,12 @@ getObjectDescription(const ObjectAddress *object, bool missing_ok)
 										 _("default privileges on new schemas belonging to role %s"),
 										 rolename);
 						break;
+					case DEFACLOBJ_LARGEOBJECT:
+						Assert(!nspname);
+						appendStringInfo(&buffer,
+										 _("default privileges on new large objects belonging to role %s"),
+										 rolename);
+						break;
 					default:
 						/* shouldn't get here */
 						if (nspname)
@@ -5720,6 +5730,10 @@ getObjectIdentityParts(const ObjectAddress *object,
 						appendStringInfoString(&buffer,
 											   " on schemas");
 						break;
+					case DEFACLOBJ_LARGEOBJECT:
+						appendStringInfoString(&buffer,
+											   " on large objects");
+						break;
 				}
 
 				if (objname)
diff --git a/src/backend/catalog/pg_largeobject.c b/src/backend/catalog/pg_largeobject.c
index 5d9fdfbd4c..ddadb3224a 100644
--- a/src/backend/catalog/pg_largeobject.c
+++ b/src/backend/catalog/pg_largeobject.c
@@ -20,6 +20,7 @@
 #include "catalog/pg_largeobject.h"
 #include "catalog/pg_largeobject_metadata.h"
 #include "miscadmin.h"
+#include "utils/acl.h"
 #include "utils/fmgroids.h"
 #include "utils/rel.h"
 
@@ -39,6 +40,8 @@ LargeObjectCreate(Oid loid)
 	Oid			loid_new;
 	Datum		values[Natts_pg_largeobject_metadata];
 	bool		nulls[Natts_pg_largeobject_metadata];
+	Oid			ownerId;
+	Acl			*lomacl;
 
 	pg_lo_meta = table_open(LargeObjectMetadataRelationId,
 							RowExclusiveLock);
@@ -55,11 +58,18 @@ LargeObjectCreate(Oid loid)
 		loid_new = GetNewOidWithIndex(pg_lo_meta,
 									  LargeObjectMetadataOidIndexId,
 									  Anum_pg_largeobject_metadata_oid);
+	ownerId = GetUserId();
+	lomacl = get_user_default_acl(OBJECT_LARGEOBJECT, ownerId, InvalidOid);
 
 	values[Anum_pg_largeobject_metadata_oid - 1] = ObjectIdGetDatum(loid_new);
 	values[Anum_pg_largeobject_metadata_lomowner - 1]
-		= ObjectIdGetDatum(GetUserId());
-	nulls[Anum_pg_largeobject_metadata_lomacl - 1] = true;
+		= ObjectIdGetDatum(ownerId);
+
+	if (lomacl != NULL)
+		values[Anum_pg_largeobject_metadata_lomacl - 1]
+			= PointerGetDatum(lomacl);
+	else
+		nulls[Anum_pg_largeobject_metadata_lomacl - 1] = true;
 
 	ntup = heap_form_tuple(RelationGetDescr(pg_lo_meta),
 						   values, nulls);
@@ -70,6 +80,10 @@ LargeObjectCreate(Oid loid)
 
 	table_close(pg_lo_meta, RowExclusiveLock);
 
+	/* dependencies on roles mentioned in default ACL */
+	recordDependencyOnNewAcl(LargeObjectRelationId, loid_new, 0,
+							 ownerId, lomacl);
+
 	return loid_new;
 }
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 84cef57a70..a77df8f028 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -756,7 +756,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
 	NULLS_P NUMERIC
 
-	OBJECT_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
+	OBJECT_P OBJECTS_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
 	ORDER ORDINALITY OTHERS OUT_P OUTER_P
 	OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER
 
@@ -8054,6 +8054,7 @@ defacl_privilege_target:
 			| SEQUENCES		{ $$ = OBJECT_SEQUENCE; }
 			| TYPES_P		{ $$ = OBJECT_TYPE; }
 			| SCHEMAS		{ $$ = OBJECT_SCHEMA; }
+			| LARGE_P OBJECTS_P	{ $$ = OBJECT_LARGEOBJECT; }
 		;
 
 
@@ -17672,6 +17673,7 @@ unreserved_keyword:
 			| NOWAIT
 			| NULLS_P
 			| OBJECT_P
+			| OBJECTS_P
 			| OF
 			| OFF
 			| OIDS
@@ -18291,6 +18293,7 @@ bare_label_keyword:
 			| NULLS_P
 			| NUMERIC
 			| OBJECT_P
+			| OBJECTS_P
 			| OF
 			| OFF
 			| OIDS
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 5649859aa1..0fad29cbaf 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -506,7 +506,8 @@ do { \
 		CONVERT_PRIV('s', "SET");
 		CONVERT_PRIV('A', "ALTER SYSTEM");
 	}
-	else if (strcmp(type, "LARGE OBJECT") == 0)
+	else if (strcmp(type, "LARGE OBJECT") == 0 ||
+			 strcmp(type, "LARGE OBJECTS") == 0)
 	{
 		CONVERT_PRIV('r', "SELECT");
 		CONVERT_PRIV('w', "UPDATE");
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 546e7e4ce1..97484a1b1b 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15009,6 +15009,9 @@ dumpDefaultACL(Archive *fout, const DefaultACLInfo *daclinfo)
 		case DEFACLOBJ_NAMESPACE:
 			type = "SCHEMAS";
 			break;
+		case DEFACLOBJ_LARGEOBJECT:
+			type = "LARGE OBJECTS";
+			break;
 		default:
 			/* shouldn't get here */
 			pg_fatal("unrecognized object type in default privileges: %d",
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 7c9a1f234c..ee1b96b0b9 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1184,7 +1184,9 @@ listDefaultACLs(const char *pattern)
 	printfPQExpBuffer(&buf,
 					  "SELECT pg_catalog.pg_get_userbyid(d.defaclrole) AS \"%s\",\n"
 					  "  n.nspname AS \"%s\",\n"
-					  "  CASE d.defaclobjtype WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' END AS \"%s\",\n"
+					  "  CASE d.defaclobjtype "
+					  "    WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s'"
+					  "    WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' END AS \"%s\",\n"
 					  "  ",
 					  gettext_noop("Owner"),
 					  gettext_noop("Schema"),
@@ -1198,6 +1200,8 @@ listDefaultACLs(const char *pattern)
 					  gettext_noop("type"),
 					  DEFACLOBJ_NAMESPACE,
 					  gettext_noop("schema"),
+					  DEFACLOBJ_LARGEOBJECT,
+					  gettext_noop("large object"),
 					  gettext_noop("Type"));
 
 	printACLColumn(&buf, "d.defaclacl");
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a7ccde6d7d..7979fd32c6 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4032,7 +4032,7 @@ psql_completion(const char *text, int start, int end)
 		 * objects supported.
 		 */
 		if (HeadMatches("ALTER", "DEFAULT", "PRIVILEGES"))
-			COMPLETE_WITH("TABLES", "SEQUENCES", "FUNCTIONS", "PROCEDURES", "ROUTINES", "TYPES", "SCHEMAS");
+			COMPLETE_WITH("TABLES", "SEQUENCES", "FUNCTIONS", "PROCEDURES", "ROUTINES", "TYPES", "SCHEMAS", "LARGE OBJECTS");
 		else
 			COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_grantables,
 											"ALL FUNCTIONS IN SCHEMA",
diff --git a/src/include/catalog/pg_default_acl.h b/src/include/catalog/pg_default_acl.h
index d272cdf08b..f9f002fa45 100644
--- a/src/include/catalog/pg_default_acl.h
+++ b/src/include/catalog/pg_default_acl.h
@@ -68,6 +68,7 @@ MAKE_SYSCACHE(DEFACLROLENSPOBJ, pg_default_acl_role_nsp_obj_index, 8);
 #define DEFACLOBJ_FUNCTION		'f' /* function */
 #define DEFACLOBJ_TYPE			'T' /* type */
 #define DEFACLOBJ_NAMESPACE		'n' /* namespace */
+#define DEFACLOBJ_LARGEOBJECT	'L' /* large object */
 
 #endif							/* EXPOSE_TO_CLIENT_CODE */
 
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index f8659078ce..cbc7f2e870 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -307,6 +307,7 @@ PG_KEYWORD("nullif", NULLIF, COL_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("nulls", NULLS_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("numeric", NUMERIC, COL_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("object", OBJECT_P, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("objects", OBJECTS_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("of", OF, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("off", OFF, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("offset", OFFSET, RESERVED_KEYWORD, AS_LABEL)
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 1d903babd3..270d82112b 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2563,11 +2563,103 @@ SELECT has_schema_privilege('regress_priv_user2', 'testns4', 'CREATE'); -- yes
 
 ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 COMMIT;
+--
+-- Test for default privileges on large objects. This is done in a
+-- separate, rollbacked, transaction to avoid any trouble with other
+-- regression sessions.
+--
+BEGIN;
+SELECT lo_create(1007);
+ lo_create 
+-----------
+      1007
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'SELECT'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'UPDATE'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1008);
+ lo_create 
+-----------
+      1008
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'SELECT'); -- yes
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user6', 1008, 'SELECT'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'UPDATE'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1009);
+ lo_create 
+-----------
+      1009
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'SELECT'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'UPDATE'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+ALTER DEFAULT PRIVILEGES REVOKE UPDATE ON LARGE OBJECTS FROM regress_priv_user2;
+SELECT lo_create(1010);
+ lo_create 
+-----------
+      1010
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'SELECT'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'UPDATE'); -- false
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ROLLBACK;
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON LARGE OBJECTS TO public; -- error
+ERROR:  cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS
+\c -
 -- Test for DROP OWNED BY with shared dependencies.  This is done in a
 -- separate, rollbacked, transaction to avoid any trouble with other
 -- regression sessions.
 BEGIN;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON FUNCTIONS TO regress_priv_user2;
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SCHEMAS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SEQUENCES TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO regress_priv_user2;
@@ -2578,7 +2670,7 @@ SELECT count(*) FROM pg_shdepend
 	classid = 'pg_default_acl'::regclass;
  count 
 -------
-     5
+     6
 (1 row)
 
 DROP OWNED BY regress_priv_user2, regress_priv_user2;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 3f54b0f8f0..5598410e44 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1535,11 +1535,47 @@ ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 
 COMMIT;
 
+--
+-- Test for default privileges on large objects. This is done in a
+-- separate, rollbacked, transaction to avoid any trouble with other
+-- regression sessions.
+--
+
+BEGIN;
+
+SELECT lo_create(1007);
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'SELECT'); -- no
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'UPDATE'); -- no
+
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON LARGE OBJECTS TO regress_priv_user2;
+
+SELECT lo_create(1008);
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'SELECT'); -- yes
+SELECT has_largeobject_privilege('regress_priv_user6', 1008, 'SELECT'); -- no
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'UPDATE'); -- no
+
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1009);
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'SELECT'); -- true
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'UPDATE'); -- true
+
+ALTER DEFAULT PRIVILEGES REVOKE UPDATE ON LARGE OBJECTS FROM regress_priv_user2;
+SELECT lo_create(1010);
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'SELECT'); -- true
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'UPDATE'); -- false
+
+ROLLBACK;
+
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON LARGE OBJECTS TO public; -- error
+
+\c -
+
 -- Test for DROP OWNED BY with shared dependencies.  This is done in a
 -- separate, rollbacked, transaction to avoid any trouble with other
 -- regression sessions.
 BEGIN;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON FUNCTIONS TO regress_priv_user2;
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SCHEMAS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SEQUENCES TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO regress_priv_user2;
-- 
2.34.1

Laurenz Albe

laurenz.albe@cybertec.at

12 months ago

In reply to: Yugo Nagata (#8)

1 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:

I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.

When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.

Attached is a rebased patch.

I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.

So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.

Yours,
Laurenz Albe

Attachments:

v4-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-patch; charset=UTF-8; name=v4-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload

From 55c0ed8c09b8bd83ced894a349c01f84b7c47e82 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Wed, 22 Jan 2025 13:15:27 +0100
Subject: [PATCH v4] Extend ALTER DEFAULT PRIVILEGES for large objects

Previously, ALTER DEFAULT PRIVILEGE didn't support large objects,
so if we want to allow users other than the owner to use the large
object, we need to grant a privilege on it every time a large object
is created.

Author: Haruka Takatsuka
Author: Yugo Nagata
Discussion: https://postgr.es/m/20240424115242.236b499b2bed5b7a27f7a418%40sraoss.co.jp
---
 doc/src/sgml/catalogs.sgml                    |  3 +-
 .../sgml/ref/alter_default_privileges.sgml    | 15 ++-
 src/backend/catalog/aclchk.c                  | 21 +++++
 src/backend/catalog/objectaddress.c           | 18 +++-
 src/backend/catalog/pg_largeobject.c          | 18 +++-
 src/backend/parser/gram.y                     |  5 +-
 src/bin/pg_dump/dumputils.c                   |  3 +-
 src/bin/pg_dump/pg_dump.c                     |  3 +
 src/bin/psql/describe.c                       |  6 +-
 src/bin/psql/tab-complete.in.c                |  2 +-
 src/include/catalog/pg_default_acl.h          |  1 +
 src/include/parser/kwlist.h                   |  1 +
 src/test/regress/expected/privileges.out      | 94 ++++++++++++++++++-
 src/test/regress/sql/privileges.sql           | 36 +++++++
 14 files changed, 215 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index d3036c5ba9d..47c185916aa 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -3338,7 +3338,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
        <literal>S</literal> = sequence,
        <literal>f</literal> = function,
        <literal>T</literal> = type,
-       <literal>n</literal> = schema
+       <literal>n</literal> = schema,
+       <literal>L</literal> = large object
       </para></entry>
      </row>
 
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 89aacec4fab..3ab695892da 100644
--- a/doc/src/sgml/ref/alter_default_privileges.sgml
+++ b/doc/src/sgml/ref/alter_default_privileges.sgml
@@ -51,6 +51,11 @@ GRANT { { USAGE | CREATE }
     ON SCHEMAS
     TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
 
+GRANT { { SELECT | UPDATE }
+    [, ...] | ALL [ PRIVILEGES ] }
+    ON LARGE OBJECTS
+    TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
 REVOKE [ GRANT OPTION FOR ]
     { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
     [, ...] | ALL [ PRIVILEGES ] }
@@ -83,6 +88,13 @@ REVOKE [ GRANT OPTION FOR ]
     ON SCHEMAS
     FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
     [ CASCADE | RESTRICT ]
+
+REVOKE [ GRANT OPTION FOR ]
+    { { SELECT | UPDATE }
+    [, ...] | ALL [ PRIVILEGES ] }
+    ON LARGE OBJECTS
+    FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
+    [ CASCADE | RESTRICT ]
 </synopsis>
  </refsynopsisdiv>
 
@@ -161,7 +173,8 @@ REVOKE [ GRANT OPTION FOR ]
       If <literal>IN SCHEMA</literal> is omitted, the global default privileges
       are altered.
       <literal>IN SCHEMA</literal> is not allowed when setting privileges
-      for schemas, since schemas can't be nested.
+      for schemas and large objects, since schemas can't be nested and
+      large objects don't belong to a schema.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 02a754cc30a..9ca8a88dc91 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -1005,6 +1005,10 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
 			all_privileges = ACL_ALL_RIGHTS_SCHEMA;
 			errormsg = gettext_noop("invalid privilege type %s for schema");
 			break;
+		case OBJECT_LARGEOBJECT:
+			all_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+			errormsg = gettext_noop("invalid privilege type %s for large object");
+			break;
 		default:
 			elog(ERROR, "unrecognized GrantStmt.objtype: %d",
 				 (int) action->objtype);
@@ -1196,6 +1200,16 @@ SetDefaultACL(InternalDefaultACL *iacls)
 				this_privileges = ACL_ALL_RIGHTS_SCHEMA;
 			break;
 
+		case OBJECT_LARGEOBJECT:
+			if (OidIsValid(iacls->nspid))
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+						 errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS")));
+			objtype = DEFACLOBJ_LARGEOBJECT;
+			if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+				this_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+			break;
+
 		default:
 			elog(ERROR, "unrecognized object type: %d",
 				 (int) iacls->objtype);
@@ -1439,6 +1453,9 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
 			case DEFACLOBJ_NAMESPACE:
 				iacls.objtype = OBJECT_SCHEMA;
 				break;
+			case DEFACLOBJ_LARGEOBJECT:
+				iacls.objtype = OBJECT_LARGEOBJECT;
+				break;
 			default:
 				/* Shouldn't get here */
 				elog(ERROR, "unexpected default ACL type: %d",
@@ -4250,6 +4267,10 @@ get_user_default_acl(ObjectType objtype, Oid ownerId, Oid nsp_oid)
 			defaclobjtype = DEFACLOBJ_NAMESPACE;
 			break;
 
+		case OBJECT_LARGEOBJECT:
+			defaclobjtype = DEFACLOBJ_LARGEOBJECT;
+			break;
+
 		default:
 			return NULL;
 	}
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d8eb8d3deaa..b63fd57dc04 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2005,16 +2005,20 @@ get_object_address_defacl(List *object, bool missing_ok)
 		case DEFACLOBJ_NAMESPACE:
 			objtype_str = "schemas";
 			break;
+		case DEFACLOBJ_LARGEOBJECT:
+			objtype_str = "large objects";
+			break;
 		default:
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					 errmsg("unrecognized default ACL object type \"%c\"", objtype),
-					 errhint("Valid object types are \"%c\", \"%c\", \"%c\", \"%c\", \"%c\".",
+					 errhint("Valid object types are \"%c\", \"%c\", \"%c\", \"%c\", \"%c\", \"%c\".",
 							 DEFACLOBJ_RELATION,
 							 DEFACLOBJ_SEQUENCE,
 							 DEFACLOBJ_FUNCTION,
 							 DEFACLOBJ_TYPE,
-							 DEFACLOBJ_NAMESPACE)));
+							 DEFACLOBJ_NAMESPACE,
+							 DEFACLOBJ_LARGEOBJECT)));
 	}
 
 	/*
@@ -3844,6 +3848,12 @@ getObjectDescription(const ObjectAddress *object, bool missing_ok)
 										 _("default privileges on new schemas belonging to role %s"),
 										 rolename);
 						break;
+					case DEFACLOBJ_LARGEOBJECT:
+						Assert(!nspname);
+						appendStringInfo(&buffer,
+										 _("default privileges on new large objects belonging to role %s"),
+										 rolename);
+						break;
 					default:
 						/* shouldn't get here */
 						if (nspname)
@@ -5766,6 +5776,10 @@ getObjectIdentityParts(const ObjectAddress *object,
 						appendStringInfoString(&buffer,
 											   " on schemas");
 						break;
+					case DEFACLOBJ_LARGEOBJECT:
+						appendStringInfoString(&buffer,
+											   " on large objects");
+						break;
 				}
 
 				if (objname)
diff --git a/src/backend/catalog/pg_largeobject.c b/src/backend/catalog/pg_largeobject.c
index 0a477a8e8a9..71a9cc134e1 100644
--- a/src/backend/catalog/pg_largeobject.c
+++ b/src/backend/catalog/pg_largeobject.c
@@ -20,6 +20,7 @@
 #include "catalog/pg_largeobject.h"
 #include "catalog/pg_largeobject_metadata.h"
 #include "miscadmin.h"
+#include "utils/acl.h"
 #include "utils/fmgroids.h"
 #include "utils/rel.h"
 
@@ -39,6 +40,8 @@ LargeObjectCreate(Oid loid)
 	Oid			loid_new;
 	Datum		values[Natts_pg_largeobject_metadata];
 	bool		nulls[Natts_pg_largeobject_metadata];
+	Oid			ownerId;
+	Acl			*lomacl;
 
 	pg_lo_meta = table_open(LargeObjectMetadataRelationId,
 							RowExclusiveLock);
@@ -55,11 +58,18 @@ LargeObjectCreate(Oid loid)
 		loid_new = GetNewOidWithIndex(pg_lo_meta,
 									  LargeObjectMetadataOidIndexId,
 									  Anum_pg_largeobject_metadata_oid);
+	ownerId = GetUserId();
+	lomacl = get_user_default_acl(OBJECT_LARGEOBJECT, ownerId, InvalidOid);
 
 	values[Anum_pg_largeobject_metadata_oid - 1] = ObjectIdGetDatum(loid_new);
 	values[Anum_pg_largeobject_metadata_lomowner - 1]
-		= ObjectIdGetDatum(GetUserId());
-	nulls[Anum_pg_largeobject_metadata_lomacl - 1] = true;
+		= ObjectIdGetDatum(ownerId);
+
+	if (lomacl != NULL)
+		values[Anum_pg_largeobject_metadata_lomacl - 1]
+			= PointerGetDatum(lomacl);
+	else
+		nulls[Anum_pg_largeobject_metadata_lomacl - 1] = true;
 
 	ntup = heap_form_tuple(RelationGetDescr(pg_lo_meta),
 						   values, nulls);
@@ -70,6 +80,10 @@ LargeObjectCreate(Oid loid)
 
 	table_close(pg_lo_meta, RowExclusiveLock);
 
+	/* dependencies on roles mentioned in default ACL */
+	recordDependencyOnNewAcl(LargeObjectRelationId, loid_new, 0,
+							 ownerId, lomacl);
+
 	return loid_new;
 }
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 6079de70e09..9bab74ebb39 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -746,7 +746,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
 	NULLS_P NUMERIC
 
-	OBJECT_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
+	OBJECT_P OBJECTS_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
 	ORDER ORDINALITY OTHERS OUT_P OUTER_P
 	OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER
 
@@ -8141,6 +8141,7 @@ defacl_privilege_target:
 			| SEQUENCES		{ $$ = OBJECT_SEQUENCE; }
 			| TYPES_P		{ $$ = OBJECT_TYPE; }
 			| SCHEMAS		{ $$ = OBJECT_SCHEMA; }
+			| LARGE_P OBJECTS_P	{ $$ = OBJECT_LARGEOBJECT; }
 		;
 
 
@@ -17809,6 +17810,7 @@ unreserved_keyword:
 			| NOWAIT
 			| NULLS_P
 			| OBJECT_P
+			| OBJECTS_P
 			| OF
 			| OFF
 			| OIDS
@@ -18430,6 +18432,7 @@ bare_label_keyword:
 			| NULLS_P
 			| NUMERIC
 			| OBJECT_P
+			| OBJECTS_P
 			| OF
 			| OFF
 			| OIDS
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 5ae77f76367..ab0e9e6da3c 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -506,7 +506,8 @@ do { \
 		CONVERT_PRIV('s', "SET");
 		CONVERT_PRIV('A', "ALTER SYSTEM");
 	}
-	else if (strcmp(type, "LARGE OBJECT") == 0)
+	else if (strcmp(type, "LARGE OBJECT") == 0 ||
+			 strcmp(type, "LARGE OBJECTS") == 0)
 	{
 		CONVERT_PRIV('r', "SELECT");
 		CONVERT_PRIV('w', "UPDATE");
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 8f73a5df956..67c2540cbc6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15195,6 +15195,9 @@ dumpDefaultACL(Archive *fout, const DefaultACLInfo *daclinfo)
 		case DEFACLOBJ_NAMESPACE:
 			type = "SCHEMAS";
 			break;
+		case DEFACLOBJ_LARGEOBJECT:
+			type = "LARGE OBJECTS";
+			break;
 		default:
 			/* shouldn't get here */
 			pg_fatal("unrecognized object type in default privileges: %d",
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2ef99971ac0..4538ea89e17 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1221,7 +1221,9 @@ listDefaultACLs(const char *pattern)
 	printfPQExpBuffer(&buf,
 					  "SELECT pg_catalog.pg_get_userbyid(d.defaclrole) AS \"%s\",\n"
 					  "  n.nspname AS \"%s\",\n"
-					  "  CASE d.defaclobjtype WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' END AS \"%s\",\n"
+					  "  CASE d.defaclobjtype "
+					  "    WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s'"
+					  "    WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' END AS \"%s\",\n"
 					  "  ",
 					  gettext_noop("Owner"),
 					  gettext_noop("Schema"),
@@ -1235,6 +1237,8 @@ listDefaultACLs(const char *pattern)
 					  gettext_noop("type"),
 					  DEFACLOBJ_NAMESPACE,
 					  gettext_noop("schema"),
+					  DEFACLOBJ_LARGEOBJECT,
+					  gettext_noop("large object"),
 					  gettext_noop("Type"));
 
 	printACLColumn(&buf, "d.defaclacl");
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 81cbf10aa28..600097da259 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4424,7 +4424,7 @@ match_previous_words(int pattern_id,
 		 * objects supported.
 		 */
 		if (HeadMatches("ALTER", "DEFAULT", "PRIVILEGES"))
-			COMPLETE_WITH("TABLES", "SEQUENCES", "FUNCTIONS", "PROCEDURES", "ROUTINES", "TYPES", "SCHEMAS");
+			COMPLETE_WITH("TABLES", "SEQUENCES", "FUNCTIONS", "PROCEDURES", "ROUTINES", "TYPES", "SCHEMAS", "LARGE OBJECTS");
 		else
 			COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_grantables,
 											"ALL FUNCTIONS IN SCHEMA",
diff --git a/src/include/catalog/pg_default_acl.h b/src/include/catalog/pg_default_acl.h
index 728024b1fa7..ce6e5098eaf 100644
--- a/src/include/catalog/pg_default_acl.h
+++ b/src/include/catalog/pg_default_acl.h
@@ -68,6 +68,7 @@ MAKE_SYSCACHE(DEFACLROLENSPOBJ, pg_default_acl_role_nsp_obj_index, 8);
 #define DEFACLOBJ_FUNCTION		'f' /* function */
 #define DEFACLOBJ_TYPE			'T' /* type */
 #define DEFACLOBJ_NAMESPACE		'n' /* namespace */
+#define DEFACLOBJ_LARGEOBJECT	'L' /* large object */
 
 #endif							/* EXPOSE_TO_CLIENT_CODE */
 
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index cf2917ad07e..58e769bce1b 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -308,6 +308,7 @@ PG_KEYWORD("nullif", NULLIF, COL_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("nulls", NULLS_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("numeric", NUMERIC, COL_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("object", OBJECT_P, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("objects", OBJECTS_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("of", OF, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("off", OFF, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("offset", OFFSET, RESERVED_KEYWORD, AS_LABEL)
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 6b01313101b..cf575b46bc8 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2637,11 +2637,103 @@ SELECT has_schema_privilege('regress_priv_user2', 'testns4', 'CREATE'); -- yes
 
 ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 COMMIT;
+--
+-- Test for default privileges on large objects. This is done in a
+-- separate, rollbacked, transaction to avoid any trouble with other
+-- regression sessions.
+--
+BEGIN;
+SELECT lo_create(1007);
+ lo_create 
+-----------
+      1007
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'SELECT'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'UPDATE'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1008);
+ lo_create 
+-----------
+      1008
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'SELECT'); -- yes
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user6', 1008, 'SELECT'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'UPDATE'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1009);
+ lo_create 
+-----------
+      1009
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'SELECT'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'UPDATE'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+ALTER DEFAULT PRIVILEGES REVOKE UPDATE ON LARGE OBJECTS FROM regress_priv_user2;
+SELECT lo_create(1010);
+ lo_create 
+-----------
+      1010
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'SELECT'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'UPDATE'); -- false
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ROLLBACK;
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON LARGE OBJECTS TO public; -- error
+ERROR:  cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS
+\c -
 -- Test for DROP OWNED BY with shared dependencies.  This is done in a
 -- separate, rollbacked, transaction to avoid any trouble with other
 -- regression sessions.
 BEGIN;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON FUNCTIONS TO regress_priv_user2;
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SCHEMAS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SEQUENCES TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO regress_priv_user2;
@@ -2652,7 +2744,7 @@ SELECT count(*) FROM pg_shdepend
 	classid = 'pg_default_acl'::regclass;
  count 
 -------
-     5
+     6
 (1 row)
 
 DROP OWNED BY regress_priv_user2, regress_priv_user2;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 60e7443bf59..57c043a0f7d 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1571,11 +1571,47 @@ ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 
 COMMIT;
 
+--
+-- Test for default privileges on large objects. This is done in a
+-- separate, rollbacked, transaction to avoid any trouble with other
+-- regression sessions.
+--
+
+BEGIN;
+
+SELECT lo_create(1007);
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'SELECT'); -- no
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'UPDATE'); -- no
+
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON LARGE OBJECTS TO regress_priv_user2;
+
+SELECT lo_create(1008);
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'SELECT'); -- yes
+SELECT has_largeobject_privilege('regress_priv_user6', 1008, 'SELECT'); -- no
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'UPDATE'); -- no
+
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1009);
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'SELECT'); -- true
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'UPDATE'); -- true
+
+ALTER DEFAULT PRIVILEGES REVOKE UPDATE ON LARGE OBJECTS FROM regress_priv_user2;
+SELECT lo_create(1010);
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'SELECT'); -- true
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'UPDATE'); -- false
+
+ROLLBACK;
+
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON LARGE OBJECTS TO public; -- error
+
+\c -
+
 -- Test for DROP OWNED BY with shared dependencies.  This is done in a
 -- separate, rollbacked, transaction to avoid any trouble with other
 -- regression sessions.
 BEGIN;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON FUNCTIONS TO regress_priv_user2;
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SCHEMAS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SEQUENCES TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO regress_priv_user2;
-- 
2.48.1

#10

nagata@sraoss.co.jp

12 months ago

In reply to: Laurenz Albe (#9)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Wed, 22 Jan 2025 13:30:17 +0100
Laurenz Albe <laurenz.albe@cybertec.at> wrote:

On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:

I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.

When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.

Attached is a rebased patch.

Thank you for updating the patch!

I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.

So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.

I confirmed the patch and I am fine with it.

Regards,
Yugo Nagata

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

#11

masao.fujii@oss.nttdata.com

10 months ago

In reply to: Yugo NAGATA (#10)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On 2025/01/23 19:22, Yugo NAGATA wrote:

On Wed, 22 Jan 2025 13:30:17 +0100
Laurenz Albe <laurenz.albe@cybertec.at> wrote:

On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:

I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.

When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.

Attached is a rebased patch.

Thank you for updating the patch!

I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.

So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.

I confirmed the patch and I am fine with it.

I've started reviewing this patch since it's marked as "ready for committer".

I know of several systems that use large objects, and I believe
this feature would be beneficial for them. Overall, I like the idea.

The latest patch looks good to me. I just have one minor comment:

only the privileges for schemas, tables (including views and foreign
tables), sequences, functions, and types (including domains) can be
altered.

In alter_default_privileges.sgml, this part should also mention large objects?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#12

nagata@sraoss.co.jp

9 months ago

In reply to: Fujii Masao (#11)

1 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Wed, 2 Apr 2025 02:35:35 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/01/23 19:22, Yugo NAGATA wrote:

On Wed, 22 Jan 2025 13:30:17 +0100
Laurenz Albe <laurenz.albe@cybertec.at> wrote:

On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:

I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.

When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.

Attached is a rebased patch.

Thank you for updating the patch!

I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.

So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.

I confirmed the patch and I am fine with it.

I've started reviewing this patch since it's marked as "ready for committer".

Thank you for your reviewing this patch!

I know of several systems that use large objects, and I believe
this feature would be beneficial for them. Overall, I like the idea.

The latest patch looks good to me. I just have one minor comment:

only the privileges for schemas, tables (including views and foreign
tables), sequences, functions, and types (including domains) can be
altered.

In alter_default_privileges.sgml, this part should also mention large objects?

Agreed. I attached a updated patch.

Regards,
Yugo Nagata

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

Attachments:

v5-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchtext/x-diff; name=v5-0001-Extend-ALTER-DEFAULT-PRIVILEGES-for-large-objects.patchDownload

From 98393b8609a97a33f1cf5ed69cb8cc28d07b25df Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Fri, 8 Mar 2024 17:43:43 +0900
Subject: [PATCH v5] Extend ALTER DEFAULT PRIVILEGES for large objects

Previously, ALTER DEFAULT PRIVILEGE didn't support large objects,
so if we want to allow users other than the owner to use the large
object, we need to grant a privilege on it every time a large object
is created.

Original patch by Haruka Takatsuka, some fixes and tests by
Yugo Nagata.
---
 doc/src/sgml/catalogs.sgml                    |  3 +-
 .../sgml/ref/alter_default_privileges.sgml    | 19 +++-
 src/backend/catalog/aclchk.c                  | 21 +++++
 src/backend/catalog/objectaddress.c           | 18 +++-
 src/backend/catalog/pg_largeobject.c          | 18 +++-
 src/backend/parser/gram.y                     |  5 +-
 src/bin/pg_dump/dumputils.c                   |  3 +-
 src/bin/pg_dump/pg_dump.c                     |  3 +
 src/bin/psql/describe.c                       |  6 +-
 src/bin/psql/tab-complete.in.c                |  2 +-
 src/include/catalog/pg_default_acl.h          |  1 +
 src/include/parser/kwlist.h                   |  1 +
 src/test/regress/expected/privileges.out      | 94 ++++++++++++++++++-
 src/test/regress/sql/privileges.sql           | 36 +++++++
 14 files changed, 217 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 4558f940aaf..45ba9c5118f 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -3360,7 +3360,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
        <literal>S</literal> = sequence,
        <literal>f</literal> = function,
        <literal>T</literal> = type,
-       <literal>n</literal> = schema
+       <literal>n</literal> = schema,
+       <literal>L</literal> = large object
       </para></entry>
      </row>
 
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 89aacec4fab..6acd0f1df91 100644
--- a/doc/src/sgml/ref/alter_default_privileges.sgml
+++ b/doc/src/sgml/ref/alter_default_privileges.sgml
@@ -51,6 +51,11 @@ GRANT { { USAGE | CREATE }
     ON SCHEMAS
     TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
 
+GRANT { { SELECT | UPDATE }
+    [, ...] | ALL [ PRIVILEGES ] }
+    ON LARGE OBJECTS
+    TO { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
 REVOKE [ GRANT OPTION FOR ]
     { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
     [, ...] | ALL [ PRIVILEGES ] }
@@ -83,6 +88,13 @@ REVOKE [ GRANT OPTION FOR ]
     ON SCHEMAS
     FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
     [ CASCADE | RESTRICT ]
+
+REVOKE [ GRANT OPTION FOR ]
+    { { SELECT | UPDATE }
+    [, ...] | ALL [ PRIVILEGES ] }
+    ON LARGE OBJECTS
+    FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
+    [ CASCADE | RESTRICT ]
 </synopsis>
  </refsynopsisdiv>
 
@@ -117,8 +129,8 @@ REVOKE [ GRANT OPTION FOR ]
   <para>
    Currently,
    only the privileges for schemas, tables (including views and foreign
-   tables), sequences, functions, and types (including domains) can be
-   altered.  For this command, functions include aggregates and procedures.
+   tables), sequences, functions, types (including domains), and large objects
+   can be altered.  For this command, functions include aggregates and procedures.
    The words <literal>FUNCTIONS</literal> and <literal>ROUTINES</literal> are
    equivalent in this command.  (<literal>ROUTINES</literal> is preferred
    going forward as the standard term for functions and procedures taken
@@ -161,7 +173,8 @@ REVOKE [ GRANT OPTION FOR ]
       If <literal>IN SCHEMA</literal> is omitted, the global default privileges
       are altered.
       <literal>IN SCHEMA</literal> is not allowed when setting privileges
-      for schemas, since schemas can't be nested.
+      for schemas and large objects, since schemas can't be nested and
+      large objects don't belong to a schema.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 02a754cc30a..9ca8a88dc91 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -1005,6 +1005,10 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
 			all_privileges = ACL_ALL_RIGHTS_SCHEMA;
 			errormsg = gettext_noop("invalid privilege type %s for schema");
 			break;
+		case OBJECT_LARGEOBJECT:
+			all_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+			errormsg = gettext_noop("invalid privilege type %s for large object");
+			break;
 		default:
 			elog(ERROR, "unrecognized GrantStmt.objtype: %d",
 				 (int) action->objtype);
@@ -1196,6 +1200,16 @@ SetDefaultACL(InternalDefaultACL *iacls)
 				this_privileges = ACL_ALL_RIGHTS_SCHEMA;
 			break;
 
+		case OBJECT_LARGEOBJECT:
+			if (OidIsValid(iacls->nspid))
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+						 errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS")));
+			objtype = DEFACLOBJ_LARGEOBJECT;
+			if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+				this_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+			break;
+
 		default:
 			elog(ERROR, "unrecognized object type: %d",
 				 (int) iacls->objtype);
@@ -1439,6 +1453,9 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
 			case DEFACLOBJ_NAMESPACE:
 				iacls.objtype = OBJECT_SCHEMA;
 				break;
+			case DEFACLOBJ_LARGEOBJECT:
+				iacls.objtype = OBJECT_LARGEOBJECT;
+				break;
 			default:
 				/* Shouldn't get here */
 				elog(ERROR, "unexpected default ACL type: %d",
@@ -4250,6 +4267,10 @@ get_user_default_acl(ObjectType objtype, Oid ownerId, Oid nsp_oid)
 			defaclobjtype = DEFACLOBJ_NAMESPACE;
 			break;
 
+		case OBJECT_LARGEOBJECT:
+			defaclobjtype = DEFACLOBJ_LARGEOBJECT;
+			break;
+
 		default:
 			return NULL;
 	}
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d8eb8d3deaa..b63fd57dc04 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2005,16 +2005,20 @@ get_object_address_defacl(List *object, bool missing_ok)
 		case DEFACLOBJ_NAMESPACE:
 			objtype_str = "schemas";
 			break;
+		case DEFACLOBJ_LARGEOBJECT:
+			objtype_str = "large objects";
+			break;
 		default:
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					 errmsg("unrecognized default ACL object type \"%c\"", objtype),
-					 errhint("Valid object types are \"%c\", \"%c\", \"%c\", \"%c\", \"%c\".",
+					 errhint("Valid object types are \"%c\", \"%c\", \"%c\", \"%c\", \"%c\", \"%c\".",
 							 DEFACLOBJ_RELATION,
 							 DEFACLOBJ_SEQUENCE,
 							 DEFACLOBJ_FUNCTION,
 							 DEFACLOBJ_TYPE,
-							 DEFACLOBJ_NAMESPACE)));
+							 DEFACLOBJ_NAMESPACE,
+							 DEFACLOBJ_LARGEOBJECT)));
 	}
 
 	/*
@@ -3844,6 +3848,12 @@ getObjectDescription(const ObjectAddress *object, bool missing_ok)
 										 _("default privileges on new schemas belonging to role %s"),
 										 rolename);
 						break;
+					case DEFACLOBJ_LARGEOBJECT:
+						Assert(!nspname);
+						appendStringInfo(&buffer,
+										 _("default privileges on new large objects belonging to role %s"),
+										 rolename);
+						break;
 					default:
 						/* shouldn't get here */
 						if (nspname)
@@ -5766,6 +5776,10 @@ getObjectIdentityParts(const ObjectAddress *object,
 						appendStringInfoString(&buffer,
 											   " on schemas");
 						break;
+					case DEFACLOBJ_LARGEOBJECT:
+						appendStringInfoString(&buffer,
+											   " on large objects");
+						break;
 				}
 
 				if (objname)
diff --git a/src/backend/catalog/pg_largeobject.c b/src/backend/catalog/pg_largeobject.c
index 0a477a8e8a9..71a9cc134e1 100644
--- a/src/backend/catalog/pg_largeobject.c
+++ b/src/backend/catalog/pg_largeobject.c
@@ -20,6 +20,7 @@
 #include "catalog/pg_largeobject.h"
 #include "catalog/pg_largeobject_metadata.h"
 #include "miscadmin.h"
+#include "utils/acl.h"
 #include "utils/fmgroids.h"
 #include "utils/rel.h"
 
@@ -39,6 +40,8 @@ LargeObjectCreate(Oid loid)
 	Oid			loid_new;
 	Datum		values[Natts_pg_largeobject_metadata];
 	bool		nulls[Natts_pg_largeobject_metadata];
+	Oid			ownerId;
+	Acl			*lomacl;
 
 	pg_lo_meta = table_open(LargeObjectMetadataRelationId,
 							RowExclusiveLock);
@@ -55,11 +58,18 @@ LargeObjectCreate(Oid loid)
 		loid_new = GetNewOidWithIndex(pg_lo_meta,
 									  LargeObjectMetadataOidIndexId,
 									  Anum_pg_largeobject_metadata_oid);
+	ownerId = GetUserId();
+	lomacl = get_user_default_acl(OBJECT_LARGEOBJECT, ownerId, InvalidOid);
 
 	values[Anum_pg_largeobject_metadata_oid - 1] = ObjectIdGetDatum(loid_new);
 	values[Anum_pg_largeobject_metadata_lomowner - 1]
-		= ObjectIdGetDatum(GetUserId());
-	nulls[Anum_pg_largeobject_metadata_lomacl - 1] = true;
+		= ObjectIdGetDatum(ownerId);
+
+	if (lomacl != NULL)
+		values[Anum_pg_largeobject_metadata_lomacl - 1]
+			= PointerGetDatum(lomacl);
+	else
+		nulls[Anum_pg_largeobject_metadata_lomacl - 1] = true;
 
 	ntup = heap_form_tuple(RelationGetDescr(pg_lo_meta),
 						   values, nulls);
@@ -70,6 +80,10 @@ LargeObjectCreate(Oid loid)
 
 	table_close(pg_lo_meta, RowExclusiveLock);
 
+	/* dependencies on roles mentioned in default ACL */
+	recordDependencyOnNewAcl(LargeObjectRelationId, loid_new, 0,
+							 ownerId, lomacl);
+
 	return loid_new;
 }
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 6a094ecc54f..f1156e2fca3 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -752,7 +752,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
 	NULLS_P NUMERIC
 
-	OBJECT_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
+	OBJECT_P OBJECTS_P OF OFF OFFSET OIDS OLD OMIT ON ONLY OPERATOR OPTION OPTIONS OR
 	ORDER ORDINALITY OTHERS OUT_P OUTER_P
 	OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER
 
@@ -8177,6 +8177,7 @@ defacl_privilege_target:
 			| SEQUENCES		{ $$ = OBJECT_SEQUENCE; }
 			| TYPES_P		{ $$ = OBJECT_TYPE; }
 			| SCHEMAS		{ $$ = OBJECT_SCHEMA; }
+			| LARGE_P OBJECTS_P	{ $$ = OBJECT_LARGEOBJECT; }
 		;
 
 
@@ -17882,6 +17883,7 @@ unreserved_keyword:
 			| NOWAIT
 			| NULLS_P
 			| OBJECT_P
+			| OBJECTS_P
 			| OF
 			| OFF
 			| OIDS
@@ -18504,6 +18506,7 @@ bare_label_keyword:
 			| NULLS_P
 			| NUMERIC
 			| OBJECT_P
+			| OBJECTS_P
 			| OF
 			| OFF
 			| OIDS
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 5ae77f76367..ab0e9e6da3c 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -506,7 +506,8 @@ do { \
 		CONVERT_PRIV('s', "SET");
 		CONVERT_PRIV('A', "ALTER SYSTEM");
 	}
-	else if (strcmp(type, "LARGE OBJECT") == 0)
+	else if (strcmp(type, "LARGE OBJECT") == 0 ||
+			 strcmp(type, "LARGE OBJECTS") == 0)
 	{
 		CONVERT_PRIV('r', "SELECT");
 		CONVERT_PRIV('w', "UPDATE");
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 04c87ba8854..817cedef32c 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15679,6 +15679,9 @@ dumpDefaultACL(Archive *fout, const DefaultACLInfo *daclinfo)
 		case DEFACLOBJ_NAMESPACE:
 			type = "SCHEMAS";
 			break;
+		case DEFACLOBJ_LARGEOBJECT:
+			type = "LARGE OBJECTS";
+			break;
 		default:
 			/* shouldn't get here */
 			pg_fatal("unrecognized object type in default privileges: %d",
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e038e9dc9e2..8970677ac64 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1222,7 +1222,9 @@ listDefaultACLs(const char *pattern)
 	printfPQExpBuffer(&buf,
 					  "SELECT pg_catalog.pg_get_userbyid(d.defaclrole) AS \"%s\",\n"
 					  "  n.nspname AS \"%s\",\n"
-					  "  CASE d.defaclobjtype WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' END AS \"%s\",\n"
+					  "  CASE d.defaclobjtype "
+					  "    WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s'"
+					  "    WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' WHEN '%c' THEN '%s' END AS \"%s\",\n"
 					  "  ",
 					  gettext_noop("Owner"),
 					  gettext_noop("Schema"),
@@ -1236,6 +1238,8 @@ listDefaultACLs(const char *pattern)
 					  gettext_noop("type"),
 					  DEFACLOBJ_NAMESPACE,
 					  gettext_noop("schema"),
+					  DEFACLOBJ_LARGEOBJECT,
+					  gettext_noop("large object"),
 					  gettext_noop("Type"));
 
 	printACLColumn(&buf, "d.defaclacl");
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 98951aef82c..c916b9299a8 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4457,7 +4457,7 @@ match_previous_words(int pattern_id,
 		 * objects supported.
 		 */
 		if (HeadMatches("ALTER", "DEFAULT", "PRIVILEGES"))
-			COMPLETE_WITH("TABLES", "SEQUENCES", "FUNCTIONS", "PROCEDURES", "ROUTINES", "TYPES", "SCHEMAS");
+			COMPLETE_WITH("TABLES", "SEQUENCES", "FUNCTIONS", "PROCEDURES", "ROUTINES", "TYPES", "SCHEMAS", "LARGE OBJECTS");
 		else
 			COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_grantables,
 											"ALL FUNCTIONS IN SCHEMA",
diff --git a/src/include/catalog/pg_default_acl.h b/src/include/catalog/pg_default_acl.h
index 728024b1fa7..ce6e5098eaf 100644
--- a/src/include/catalog/pg_default_acl.h
+++ b/src/include/catalog/pg_default_acl.h
@@ -68,6 +68,7 @@ MAKE_SYSCACHE(DEFACLROLENSPOBJ, pg_default_acl_role_nsp_obj_index, 8);
 #define DEFACLOBJ_FUNCTION		'f' /* function */
 #define DEFACLOBJ_TYPE			'T' /* type */
 #define DEFACLOBJ_NAMESPACE		'n' /* namespace */
+#define DEFACLOBJ_LARGEOBJECT	'L' /* large object */
 
 #endif							/* EXPOSE_TO_CLIENT_CODE */
 
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 40cf090ce61..a4af3f717a1 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -308,6 +308,7 @@ PG_KEYWORD("nullif", NULLIF, COL_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("nulls", NULLS_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("numeric", NUMERIC, COL_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("object", OBJECT_P, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("objects", OBJECTS_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("of", OF, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("off", OFF, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("offset", OFFSET, RESERVED_KEYWORD, AS_LABEL)
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 5588d83e1bf..1fddb13b6ae 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2667,11 +2667,103 @@ SELECT has_schema_privilege('regress_priv_user2', 'testns4', 'CREATE'); -- yes
 
 ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 COMMIT;
+--
+-- Test for default privileges on large objects. This is done in a
+-- separate, rollbacked, transaction to avoid any trouble with other
+-- regression sessions.
+--
+BEGIN;
+SELECT lo_create(1007);
+ lo_create 
+-----------
+      1007
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'SELECT'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'UPDATE'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1008);
+ lo_create 
+-----------
+      1008
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'SELECT'); -- yes
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user6', 1008, 'SELECT'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'UPDATE'); -- no
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1009);
+ lo_create 
+-----------
+      1009
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'SELECT'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'UPDATE'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+ALTER DEFAULT PRIVILEGES REVOKE UPDATE ON LARGE OBJECTS FROM regress_priv_user2;
+SELECT lo_create(1010);
+ lo_create 
+-----------
+      1010
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'SELECT'); -- true
+ has_largeobject_privilege 
+---------------------------
+ t
+(1 row)
+
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'UPDATE'); -- false
+ has_largeobject_privilege 
+---------------------------
+ f
+(1 row)
+
+ROLLBACK;
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON LARGE OBJECTS TO public; -- error
+ERROR:  cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS
+\c -
 -- Test for DROP OWNED BY with shared dependencies.  This is done in a
 -- separate, rollbacked, transaction to avoid any trouble with other
 -- regression sessions.
 BEGIN;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON FUNCTIONS TO regress_priv_user2;
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SCHEMAS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SEQUENCES TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO regress_priv_user2;
@@ -2682,7 +2774,7 @@ SELECT count(*) FROM pg_shdepend
 	classid = 'pg_default_acl'::regclass;
  count 
 -------
-     5
+     6
 (1 row)
 
 DROP OWNED BY regress_priv_user2, regress_priv_user2;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 286b1d03756..85d7280f35f 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1586,11 +1586,47 @@ ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 
 COMMIT;
 
+--
+-- Test for default privileges on large objects. This is done in a
+-- separate, rollbacked, transaction to avoid any trouble with other
+-- regression sessions.
+--
+
+BEGIN;
+
+SELECT lo_create(1007);
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'SELECT'); -- no
+SELECT has_largeobject_privilege('regress_priv_user2', 1007, 'UPDATE'); -- no
+
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON LARGE OBJECTS TO regress_priv_user2;
+
+SELECT lo_create(1008);
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'SELECT'); -- yes
+SELECT has_largeobject_privilege('regress_priv_user6', 1008, 'SELECT'); -- no
+SELECT has_largeobject_privilege('regress_priv_user2', 1008, 'UPDATE'); -- no
+
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
+SELECT lo_create(1009);
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'SELECT'); -- true
+SELECT has_largeobject_privilege('regress_priv_user2', 1009, 'UPDATE'); -- true
+
+ALTER DEFAULT PRIVILEGES REVOKE UPDATE ON LARGE OBJECTS FROM regress_priv_user2;
+SELECT lo_create(1010);
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'SELECT'); -- true
+SELECT has_largeobject_privilege('regress_priv_user2', 1010, 'UPDATE'); -- false
+
+ROLLBACK;
+
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL ON LARGE OBJECTS TO public; -- error
+
+\c -
+
 -- Test for DROP OWNED BY with shared dependencies.  This is done in a
 -- separate, rollbacked, transaction to avoid any trouble with other
 -- regression sessions.
 BEGIN;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON FUNCTIONS TO regress_priv_user2;
+ALTER DEFAULT PRIVILEGES GRANT ALL ON LARGE OBJECTS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SCHEMAS TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON SEQUENCES TO regress_priv_user2;
 ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO regress_priv_user2;
-- 
2.34.1

#13

masao.fujii@oss.nttdata.com

9 months ago

In reply to: Yugo NAGATA (#12)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On 2025/04/03 23:04, Yugo NAGATA wrote:

On Wed, 2 Apr 2025 02:35:35 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/01/23 19:22, Yugo NAGATA wrote:

On Wed, 22 Jan 2025 13:30:17 +0100
Laurenz Albe <laurenz.albe@cybertec.at> wrote:

On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:

I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
function instead of calling loread & lowrite, which makes the test a bit simpler.
Thare are no other changes.

When I tried to apply this patch, I found that it doesn't apply any
more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.

Attached is a rebased patch.

Thank you for updating the patch!

I agree that large objects are a feature that should fade out (alas,
the JDBC driver still uses it for BLOBs). But this patch is not big
or complicated and is unlikely to create a big maintenance burden.

So I am somewhat for committing it. It works as advertised.
If you are fine with my rebased patch, I can mark it as "ready for
committer". If it actually gets committed depends on whether there
is a committer who thinks it worth the effort or not.

I confirmed the patch and I am fine with it.

I've started reviewing this patch since it's marked as "ready for committer".

Thank you for your reviewing this patch!

I know of several systems that use large objects, and I believe
this feature would be beneficial for them. Overall, I like the idea.

The latest patch looks good to me. I just have one minor comment:

only the privileges for schemas, tables (including views and foreign
tables), sequences, functions, and types (including domains) can be
altered.

In alter_default_privileges.sgml, this part should also mention large objects?

Agreed. I attached a updated patch.

Thanks for updating the patch!

If there are no objections, I'll proceed with committing it using the following commit log.

----------------
Extend ALTER DEFAULT PRIVILEGES to define default privileges for large objects.

Previously, ALTER DEFAULT PRIVILEGES did not support large objects.
This meant that to grant privileges to users other than the owner,
permissions had to be manually assigned each time a large object
was created, which was inconvenient.

This commit extends ALTER DEFAULT PRIVILEGES to allow defining default
access privileges for large objects. With this change, specified privileges
will automatically apply to newly created large objects, making privilege
management more efficient.

As a side effect, this commit introduces the new keyword OBJECTS
since it's used in the syntax of ALTER DEFAULT PRIVILEGES.

Original patch by Haruka Takatsuka, with some fixes and tests by Yugo Nagata,
and rebased by Laurenz Albe.

Author: Takatsuka Haruka <harukat@sraoss.co.jp>
Co-authored-by: Yugo Nagata <nagata@sraoss.co.jp>
Co-authored-by: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Masao Fujii <masao.fujii@gmail.com>
Discussion: /messages/by-id/20240424115242.236b499b2bed5b7a27f7a418@sraoss.co.jp
----------------

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#14

masao.fujii@oss.nttdata.com

9 months ago

In reply to: Fujii Masao (#13)

1 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On 2025/04/04 0:21, Fujii Masao wrote:

Thanks for updating the patch!

If there are no objections, I'll proceed with committing it using the following commit log.

I've pushed the patch. Thanks!

While testing the feature, I noticed that psql doesn't complete
"ALTER DEFAULT PRIVILEGES GRANT/REVOKE ... ON LARGE OBJECTS" or
"GRANT/REVOKE ... ON LARGE OBJECT ..." with TO/FROM. The attached
patch adds tab-completion support for both cases.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Attachments:

v1-0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE.patchtext/plain; charset=UTF-8; name=v1-0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE.patchDownload

From 02c1811af49b5f417af71b601cb60621640a14b4 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 4 Apr 2025 10:51:20 +0900
Subject: [PATCH v1] psql: Improve psql tab completion for GRANT/REVOKE on
 large objects.

This commit enhances psql's tab completion to support TO/FROM
after "GRANT/REVOKE ... ON LARGE OBJECT ...". Additionally,
since "ALTER DEFAULT PRIVILEGES" now supports large objects,
tab completion is also updated for "GRANT/REVOKE ... ON LARGE OBJECTS"
with TO/FROM.
---
 src/bin/psql/tab-complete.in.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index c916b9299a8..bdeb95fb3c8 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4602,6 +4602,26 @@ match_previous_words(int pattern_id,
 			COMPLETE_WITH("FROM");
 	}
 
+	/* Complete "GRANT/REVOKE * ON LARGE OBJECT *" with TO/FROM */
+	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "LARGE", "OBJECT", MatchAny) ||
+			 TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny, "ON", "LARGE", "OBJECT", MatchAny))
+	{
+		if (TailMatches("GRANT", MatchAny, MatchAny, MatchAny, MatchAny, MatchAny))
+			COMPLETE_WITH("TO");
+		else
+			COMPLETE_WITH("FROM");
+	}
+
+	/* Complete "GRANT/REVOKE * ON LARGE OBJECTS" with TO/FROM */
+	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "LARGE", "OBJECTS") ||
+			 TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny, "ON", "LARGE", "OBJECTS"))
+	{
+		if (TailMatches("GRANT", MatchAny, MatchAny, MatchAny, MatchAny))
+			COMPLETE_WITH("TO");
+		else
+			COMPLETE_WITH("FROM");
+	}
+
 /* GROUP BY */
 	else if (TailMatches("FROM", MatchAny, "GROUP"))
 		COMPLETE_WITH("BY");
-- 
2.48.1

#15

Nathan Bossart

nathandbossart@gmail.com

9 months ago

In reply to: Fujii Masao (#14)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Fri, Apr 04, 2025 at 07:18:11PM +0900, Fujii Masao wrote:

I've pushed the patch. Thanks!

Just a heads up, I fixed a pgindent issue in this commit (see commits
e1a8b1ad58 and 742317a80f). I'd ordinarily just report it, but since we're
nearing feature freeze, I just fixed it because my workflow (and presumably
others') involves running pgindent on the entire tree periodically.

--
nathan

#16

masao.fujii@oss.nttdata.com

9 months ago

In reply to: Nathan Bossart (#15)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On 2025/04/04 23:47, Nathan Bossart wrote:

On Fri, Apr 04, 2025 at 07:18:11PM +0900, Fujii Masao wrote:

I've pushed the patch. Thanks!

Just a heads up, I fixed a pgindent issue in this commit (see commits
e1a8b1ad58 and 742317a80f). I'd ordinarily just report it, but since we're
nearing feature freeze, I just fixed it because my workflow (and presumably
others') involves running pgindent on the entire tree periodically.

Thanks a lot!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#17

nagata@sraoss.co.jp

9 months ago

In reply to: Fujii Masao (#14)

2 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Fri, 4 Apr 2025 19:18:11 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/04/04 0:21, Fujii Masao wrote:

Thanks for updating the patch!

If there are no objections, I'll proceed with committing it using the following commit log.

I've pushed the patch. Thanks!

Thank you!

While testing the feature, I noticed that psql doesn't complete
"ALTER DEFAULT PRIVILEGES GRANT/REVOKE ... ON LARGE OBJECTS" or
"GRANT/REVOKE ... ON LARGE OBJECT ..." with TO/FROM. The attached
patch adds tab-completion support for both cases.

This patch looks good to me. This works as expected.

While looking into this patch, I found that the tab completion suggests
TO/FROM even after "LARGE OBJECT", but it is not correct because
there should be largeobject id at that place. This is same for the
"FOREIGN SERVER", server names should be suggested ratar than TO/FROM
in this case.

The additional patch 0002 fixed to prevents to suggest TO or FROM right
after LARGE OBJECT or FOREIGN SERVER. Also, it allows to suggest list of
foreign server names after FOREIGN SERVER.

The 0001 patch is the same you proposed.

Regards,
Yugo Nagata

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

Attachments:

0002-psql-Some-improvement-of-tab-completion-for-GRANT-RE.patchtext/x-diff; name=0002-psql-Some-improvement-of-tab-completion-for-GRANT-RE.patchDownload

From 64d5aead5ab080d40fa85f9bd51cb0a09490266f Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Tue, 8 Apr 2025 12:15:45 +0900
Subject: [PATCH 2/2] psql: Some improvement of tab completion for GRANT/REVOKE

This prevents to suggest TO or FROM just after LARGE OBJECT
or FOREIGN SERVER because there should be largeobject id or
server name at that place. Also, it allows to suggest list of
foreign server names after FOREIGN SERVER.
---
 src/bin/psql/tab-complete.in.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index bdeb95fb3c8..c58ed27e872 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4567,10 +4567,18 @@ match_previous_words(int pattern_id,
 	else if (Matches("ALTER", "DEFAULT", "PRIVILEGES", MatchAnyN, "TO", MatchAny))
 		COMPLETE_WITH("WITH GRANT OPTION");
 	/* Complete "GRANT/REVOKE ... ON * *" with TO/FROM */
-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+	{
+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
+		else if (!TailMatches("LARGE", "OBJECT"))
+		{
+			if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+				COMPLETE_WITH("TO");
+			else
+				COMPLETE_WITH("FROM");
+		}
+	}
 
 	/* Complete "GRANT/REVOKE * ON ALL * IN SCHEMA *" with TO/FROM */
 	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "ALL", MatchAny, "IN", "SCHEMA", MatchAny) ||
-- 
2.34.1

0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE-on.patchtext/x-diff; name=0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE-on.patchDownload

From 8402036dcc5e32a249a7af0fb9e27c31ba8b72a6 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 4 Apr 2025 10:51:20 +0900
Subject: [PATCH 1/2] psql: Improve psql tab completion for GRANT/REVOKE on
 large objects.

This commit enhances psql's tab completion to support TO/FROM
after "GRANT/REVOKE ... ON LARGE OBJECT ...". Additionally,
since "ALTER DEFAULT PRIVILEGES" now supports large objects,
tab completion is also updated for "GRANT/REVOKE ... ON LARGE OBJECTS"
with TO/FROM.
---
 src/bin/psql/tab-complete.in.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index c916b9299a8..bdeb95fb3c8 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4602,6 +4602,26 @@ match_previous_words(int pattern_id,
 			COMPLETE_WITH("FROM");
 	}
 
+	/* Complete "GRANT/REVOKE * ON LARGE OBJECT *" with TO/FROM */
+	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "LARGE", "OBJECT", MatchAny) ||
+			 TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny, "ON", "LARGE", "OBJECT", MatchAny))
+	{
+		if (TailMatches("GRANT", MatchAny, MatchAny, MatchAny, MatchAny, MatchAny))
+			COMPLETE_WITH("TO");
+		else
+			COMPLETE_WITH("FROM");
+	}
+
+	/* Complete "GRANT/REVOKE * ON LARGE OBJECTS" with TO/FROM */
+	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "LARGE", "OBJECTS") ||
+			 TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny, "ON", "LARGE", "OBJECTS"))
+	{
+		if (TailMatches("GRANT", MatchAny, MatchAny, MatchAny, MatchAny))
+			COMPLETE_WITH("TO");
+		else
+			COMPLETE_WITH("FROM");
+	}
+
 /* GROUP BY */
 	else if (TailMatches("FROM", MatchAny, "GROUP"))
 		COMPLETE_WITH("BY");
-- 
2.34.1

#18

[1]: /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com

nagata@sraoss.co.jp

7 months ago

In reply to: Yugo NAGATA (#17)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Tue, 8 Apr 2025 12:28:57 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:

On Fri, 4 Apr 2025 19:18:11 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/04/04 0:21, Fujii Masao wrote:

Thanks for updating the patch!

If there are no objections, I'll proceed with committing it using the following commit log.

I've pushed the patch. Thanks!

Thank you!

While testing the feature, I noticed that psql doesn't complete
"ALTER DEFAULT PRIVILEGES GRANT/REVOKE ... ON LARGE OBJECTS" or
"GRANT/REVOKE ... ON LARGE OBJECT ..." with TO/FROM. The attached
patch adds tab-completion support for both cases.

This patch looks good to me. This works as expected.

While looking into this patch, I found that the tab completion suggests
TO/FROM even after "LARGE OBJECT", but it is not correct because
there should be largeobject id at that place. This is same for the
"FOREIGN SERVER", server names should be suggested ratar than TO/FROM
in this case.

The additional patch 0002 fixed to prevents to suggest TO or FROM right
after LARGE OBJECT or FOREIGN SERVER. Also, it allows to suggest list of
foreign server names after FOREIGN SERVER.

While looking at the thread [1]/messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com, I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?

Best regards,
Yugo Nagata

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

#19

masao.fujii@oss.nttdata.com

7 months ago

In reply to: Yugo Nagata (#18)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On 2025/06/11 11:49, Yugo Nagata wrote:

While looking at the thread [1], I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?

[1] /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com

I see these patches more as enhancements to psql tab-completion,
rather than fixes for clear oversights in the original commit.

For example, if tab-completion for ALTER DEFAULT PRIVILEGES had
completely missed LARGE OBJECTS, that would be an obvious oversight.
But these patches go beyond that kind of issue.

That said, if others think it's appropriate to include them in v18
for consistency or completeness, I'm fine with that.

Regarding the 0002 patch:

-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+	{
+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
+		else if (!TailMatches("LARGE", "OBJECT"))
+		{
+			if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+				COMPLETE_WITH("TO");
+			else
+				COMPLETE_WITH("FROM");
+		}
+	}

Wouldn't this change break the case where "GRANT ... ON TABLE ... <TAB>"
is supposed to complete with "TO"?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

#20

nagata@sraoss.co.jp

7 months ago

In reply to: Fujii Masao (#19)

2 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Wed, 11 Jun 2025 13:33:07 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/06/11 11:49, Yugo Nagata wrote:

While looking at the thread [1], I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?

[1] /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com

I see these patches more as enhancements to psql tab-completion,
rather than fixes for clear oversights in the original commit.

For example, if tab-completion for ALTER DEFAULT PRIVILEGES had
completely missed LARGE OBJECTS, that would be an obvious oversight.
But these patches go beyond that kind of issue.

That said, if others think it's appropriate to include them in v18
for consistency or completeness, I'm fine with that.

Regarding the 0002 patch:
-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+	{
+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
+		else if (!TailMatches("LARGE", "OBJECT"))
+		{
+			if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+				COMPLETE_WITH("TO");
+			else
+				COMPLETE_WITH("FROM");
+		}
+	}
Wouldn't this change break the case where "GRANT ... ON TABLE ... <TAB>"
is supposed to complete with "TO"?

Sorry, I made a stupid mistake.

+ else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))

This should be "GRANT|REVOKE".

I've attached update patches. (There is no change on 0001.)

Best regards,
Yugo Nagata

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

Attachments:

v2-0002-psql-Some-improvement-of-tab-completion-for-GRANT.patchtext/x-diff; name=v2-0002-psql-Some-improvement-of-tab-completion-for-GRANT.patchDownload

From 31558e92e0383ebd2be7d73d56675a723d8993ec Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Tue, 8 Apr 2025 12:15:45 +0900
Subject: [PATCH v2 2/2] psql: Some improvement of tab completion for
 GRANT/REVOKE

This prevents to suggest TO or FROM just after LARGE OBJECT
or FOREIGN SERVER because there should be largeobject id or
server name at that place. Also, it allows to suggest list of
foreign server names after FOREIGN SERVER.
---
 src/bin/psql/tab-complete.in.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 42e23962bba..eebe7ac03ac 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4573,10 +4573,18 @@ match_previous_words(int pattern_id,
 	else if (Matches("ALTER", "DEFAULT", "PRIVILEGES", MatchAnyN, "TO", MatchAny))
 		COMPLETE_WITH("WITH GRANT OPTION");
 	/* Complete "GRANT/REVOKE ... ON * *" with TO/FROM */
-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT|REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+	{
+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
+		else if (!TailMatches("LARGE", "OBJECT"))
+		{
+			if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+				COMPLETE_WITH("TO");
+			else
+				COMPLETE_WITH("FROM");
+		}
+	}
 
 	/* Complete "GRANT/REVOKE * ON ALL * IN SCHEMA *" with TO/FROM */
 	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "ALL", MatchAny, "IN", "SCHEMA", MatchAny) ||
-- 
2.43.0

v2-0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE.patchtext/x-diff; name=v2-0001-psql-Improve-psql-tab-completion-for-GRANT-REVOKE.patchDownload

From 4c83dc42c9dfa9ee7cb9c3b92ca19a1ebf5bf2d9 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 4 Apr 2025 10:51:20 +0900
Subject: [PATCH v2 1/2] psql: Improve psql tab completion for GRANT/REVOKE on
 large objects.

This commit enhances psql's tab completion to support TO/FROM
after "GRANT/REVOKE ... ON LARGE OBJECT ...". Additionally,
since "ALTER DEFAULT PRIVILEGES" now supports large objects,
tab completion is also updated for "GRANT/REVOKE ... ON LARGE OBJECTS"
with TO/FROM.
---
 src/bin/psql/tab-complete.in.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index ec65ab79fec..42e23962bba 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4608,6 +4608,26 @@ match_previous_words(int pattern_id,
 			COMPLETE_WITH("FROM");
 	}
 
+	/* Complete "GRANT/REVOKE * ON LARGE OBJECT *" with TO/FROM */
+	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "LARGE", "OBJECT", MatchAny) ||
+			 TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny, "ON", "LARGE", "OBJECT", MatchAny))
+	{
+		if (TailMatches("GRANT", MatchAny, MatchAny, MatchAny, MatchAny, MatchAny))
+			COMPLETE_WITH("TO");
+		else
+			COMPLETE_WITH("FROM");
+	}
+
+	/* Complete "GRANT/REVOKE * ON LARGE OBJECTS" with TO/FROM */
+	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "LARGE", "OBJECTS") ||
+			 TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny, "ON", "LARGE", "OBJECTS"))
+	{
+		if (TailMatches("GRANT", MatchAny, MatchAny, MatchAny, MatchAny))
+			COMPLETE_WITH("TO");
+		else
+			COMPLETE_WITH("FROM");
+	}
+
 /* GROUP BY */
 	else if (TailMatches("FROM", MatchAny, "GROUP"))
 		COMPLETE_WITH("BY");
-- 
2.43.0

#21

nagata@sraoss.co.jp

7 months ago

In reply to: Fujii Masao (#19)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Wed, 11 Jun 2025 13:33:07 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/06/11 11:49, Yugo Nagata wrote:

While looking at the thread [1], I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?

[1] /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com

I see these patches more as enhancements to psql tab-completion,
rather than fixes for clear oversights in the original commit.

For example, if tab-completion for ALTER DEFAULT PRIVILEGES had
completely missed LARGE OBJECTS, that would be an obvious oversight.
But these patches go beyond that kind of issue.

Thank you for your clarification. I agreed.

Best regards,
Yugo Nagata

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

#22

masao.fujii@oss.nttdata.com

6 months ago

In reply to: Yugo Nagata (#20)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On 2025/06/11 13:57, Yugo Nagata wrote:

On Wed, 11 Jun 2025 13:33:07 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2025/06/11 11:49, Yugo Nagata wrote:

While looking at the thread [1], I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?

[1] /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com

I see these patches more as enhancements to psql tab-completion,
rather than fixes for clear oversights in the original commit.

For example, if tab-completion for ALTER DEFAULT PRIVILEGES had
completely missed LARGE OBJECTS, that would be an obvious oversight.
But these patches go beyond that kind of issue.

That said, if others think it's appropriate to include them in v18
for consistency or completeness, I'm fine with that.

Regarding the 0002 patch:
-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+	{
+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
+		else if (!TailMatches("LARGE", "OBJECT"))
+		{
+			if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+				COMPLETE_WITH("TO");
+			else
+				COMPLETE_WITH("FROM");
+		}
+	}
Wouldn't this change break the case where "GRANT ... ON TABLE ... <TAB>"
is supposed to complete with "TO"?
Sorry, I made a stupid mistake.

+ else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))

This should be "GRANT|REVOKE".

I've attached update patches. (There is no change on 0001.)

Thanks for updating the patch! At first I've pushed the 0001 patch.

As for the 0002 patch:

+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);

This part seems not needed, since we already have the following tab-completion code:

/* FOREIGN SERVER */
else if (TailMatches("FOREIGN", "SERVER"))
COMPLETE_WITH_QUERY(Query_for_list_of_servers);

Thought?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

#23

nagata@sraoss.co.jp

6 months ago

In reply to: Fujii Masao (#22)

1 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Wed, 9 Jul 2025 20:42:42 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/06/11 13:57, Yugo Nagata wrote:
On Wed, 11 Jun 2025 13:33:07 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2025/06/11 11:49, Yugo Nagata wrote:

While looking at the thread [1], I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?

[1] /messages/by-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a@oss.nttdata.com

I see these patches more as enhancements to psql tab-completion,
rather than fixes for clear oversights in the original commit.

For example, if tab-completion for ALTER DEFAULT PRIVILEGES had
completely missed LARGE OBJECTS, that would be an obvious oversight.
But these patches go beyond that kind of issue.

That said, if others think it's appropriate to include them in v18
for consistency or completeness, I'm fine with that.

Regarding the 0002 patch:
-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+	{
+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
+		else if (!TailMatches("LARGE", "OBJECT"))
+		{
+			if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+				COMPLETE_WITH("TO");
+			else
+				COMPLETE_WITH("FROM");
+		}
+	}
Wouldn't this change break the case where "GRANT ... ON TABLE ... <TAB>"
is supposed to complete with "TO"?
Sorry, I made a stupid mistake.

+ else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))

This should be "GRANT|REVOKE".

I've attached update patches. (There is no change on 0001.)
Thanks for updating the patch! At first I've pushed the 0001 patch.

As for the 0002 patch:
+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
This part seems not needed, since we already have the following tab-completion code:

/* FOREIGN SERVER */
else if (TailMatches("FOREIGN", "SERVER"))
COMPLETE_WITH_QUERY(Query_for_list_of_servers);

Thought?

You're right. I must have overlooked something. I think I saw "TO" being
suggested after "FOREIGN SERVER" when no foreign servers were defined.

The attached patch still prevents "TO/FROM" from being suggested after
"FOREIGN SERVER" in such cases. But perhaps this corner case doesn't really
need to be handled?

Regards,
Yugo Nagata

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

Attachments:

v3-0001-psql-Some-improvement-of-tab-completion-for-GRANT.patchtext/x-diff; name=v3-0001-psql-Some-improvement-of-tab-completion-for-GRANT.patchDownload

From 4ba9c4155ace519e016f26159e9587e6da488b86 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Tue, 8 Apr 2025 12:15:45 +0900
Subject: [PATCH v3] psql: Some improvement of tab completion for GRANT/REVOKE

This prevents suggesting TO or FROM immediately after LARGE OBJECT,
as a largeobject id is expected in that position. It also avoids suggesting
TO or FROM after FOREIGN SERVER when no foreign servers are defined.
When foreign servers do exist, their names are already suggested.
---
 src/bin/psql/tab-complete.in.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 5ba45a0bcb3..ec5f50c64cb 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4603,10 +4603,16 @@ match_previous_words(int pattern_id,
 	else if (Matches("ALTER", "DEFAULT", "PRIVILEGES", MatchAnyN, "TO", MatchAny))
 		COMPLETE_WITH("WITH GRANT OPTION");
 	/* Complete "GRANT/REVOKE ... ON * *" with TO/FROM */
-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT|REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+	{
+		if (!TailMatches("FOREIGN", "SERVER") && !TailMatches("LARGE", "OBJECT"))
+		{
+			if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+				COMPLETE_WITH("TO");
+			else
+				COMPLETE_WITH("FROM");
+		}
+	}
 
 	/* Complete "GRANT/REVOKE * ON ALL * IN SCHEMA *" with TO/FROM */
 	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "ALL", MatchAny, "IN", "SCHEMA", MatchAny) ||
-- 
2.43.0

#24

masao.fujii@oss.nttdata.com

6 months ago

In reply to: Yugo Nagata (#23)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On 2025/07/10 10:30, Yugo Nagata wrote:

You're right. I must have overlooked something. I think I saw "TO" being
suggested after "FOREIGN SERVER" when no foreign servers were defined.

The attached patch still prevents "TO/FROM" from being suggested after
"FOREIGN SERVER" in such cases.

Thanks for updating the patch!

Based on your patch, I'm thinking of simplifying the code like this:

-       else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-               COMPLETE_WITH("TO");
-       else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-               COMPLETE_WITH("FROM");
+       else if (Matches("GRANT|REVOKE", MatchAnyN, "ON", MatchAny, MatchAny) &&
+                        !TailMatches("FOREIGN", "SERVER") && !TailMatches("LARGE", "OBJECT"))
+       {
+               if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+                       COMPLETE_WITH("TO");
+               else
+                       COMPLETE_WITH("FROM");
+       }

But perhaps this corner case doesn't really
need to be handled?

Probably I failed to get your point here. Could you clarify what you meant?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

#25

nagata@sraoss.co.jp

6 months ago

In reply to: Fujii Masao (#24)

1 attachment(s)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Thu, 10 Jul 2025 13:23:47 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/07/10 10:30, Yugo Nagata wrote:

You're right. I must have overlooked something. I think I saw "TO" being
suggested after "FOREIGN SERVER" when no foreign servers were defined.

The attached patch still prevents "TO/FROM" from being suggested after
"FOREIGN SERVER" in such cases.

Based on your patch, I'm thinking of simplifying the code like this:

-       else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-               COMPLETE_WITH("TO");
-       else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-               COMPLETE_WITH("FROM");
+       else if (Matches("GRANT|REVOKE", MatchAnyN, "ON", MatchAny, MatchAny) &&
+                        !TailMatches("FOREIGN", "SERVER") && !TailMatches("LARGE", "OBJECT"))
+       {
+               if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+                       COMPLETE_WITH("TO");
+               else
+                       COMPLETE_WITH("FROM");
+       }

Thank you for your review!
I agree with your suggestion and have updated the patch accordingly.

But perhaps this corner case doesn't really
need to be handled?

Probably I failed to get your point here. Could you clarify what you meant?

I'm sorry for not explaining it clearly.

Currently, TO or FROM could be suggested immediately after FOREIGN SERVER, but
only when no foreign servers are defined. When foreign servers do exist,
their names are correctly suggested instead, as expected.

The patch fixed the behavior so that TO or FROM are not suggested after FOREIGN SERVER,
even when no foreign servers are defined. However, I've started to wonder if it's worth
fixing such a corner case. What do you think?

Regards,
Yugo Nagata

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

Attachments:

v4-0001-psql-Some-improvement-of-tab-completion-for-GRANT.patchtext/x-diff; name=v4-0001-psql-Some-improvement-of-tab-completion-for-GRANT.patchDownload

From 3bf87cdc51c7576833b283b97d96927dfff6a7b0 Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Tue, 8 Apr 2025 12:15:45 +0900
Subject: [PATCH v4] psql: Some improvement of tab completion for GRANT/REVOKE

This prevents suggesting TO or FROM immediately after LARGE OBJECT,
as a largeobject id is expected in that position. It also avoids suggesting
TO or FROM after FOREIGN SERVER when no foreign servers are defined.
When foreign servers do exist, their names are already suggested.
---
 src/bin/psql/tab-complete.in.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 5ba45a0bcb3..845a1894e36 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -4603,10 +4603,14 @@ match_previous_words(int pattern_id,
 	else if (Matches("ALTER", "DEFAULT", "PRIVILEGES", MatchAnyN, "TO", MatchAny))
 		COMPLETE_WITH("WITH GRANT OPTION");
 	/* Complete "GRANT/REVOKE ... ON * *" with TO/FROM */
-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT|REVOKE", MatchAnyN, "ON", MatchAny, MatchAny) &&
+			 !TailMatches("FOREIGN", "SERVER") && !TailMatches("LARGE", "OBJECT"))
+	{
+		if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+			COMPLETE_WITH("TO");
+		else
+			COMPLETE_WITH("FROM");
+	}
 
 	/* Complete "GRANT/REVOKE * ON ALL * IN SCHEMA *" with TO/FROM */
 	else if (TailMatches("GRANT|REVOKE", MatchAny, "ON", "ALL", MatchAny, "IN", "SCHEMA", MatchAny) ||
-- 
2.43.0

#26

masao.fujii@oss.nttdata.com

6 months ago

In reply to: Yugo Nagata (#25)

Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On 2025/07/10 14:11, Yugo Nagata wrote:

On Thu, 10 Jul 2025 13:23:47 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/07/10 10:30, Yugo Nagata wrote:

You're right. I must have overlooked something. I think I saw "TO" being
suggested after "FOREIGN SERVER" when no foreign servers were defined.

The attached patch still prevents "TO/FROM" from being suggested after
"FOREIGN SERVER" in such cases.
Based on your patch, I'm thinking of simplifying the code like this:
-       else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-               COMPLETE_WITH("TO");
-       else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-               COMPLETE_WITH("FROM");
+       else if (Matches("GRANT|REVOKE", MatchAnyN, "ON", MatchAny, MatchAny) &&
+                        !TailMatches("FOREIGN", "SERVER") && !TailMatches("LARGE", "OBJECT"))
+       {
+               if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+                       COMPLETE_WITH("TO");
+               else
+                       COMPLETE_WITH("FROM");
+       }
Thank you for your review!
I agree with your suggestion and have updated the patch accordingly.

I've pushed the patch. Thanks!

But perhaps this corner case doesn't really
need to be handled?

Probably I failed to get your point here. Could you clarify what you meant?

I'm sorry for not explaining it clearly.

Currently, TO or FROM could be suggested immediately after FOREIGN SERVER, but
only when no foreign servers are defined. When foreign servers do exist,
their names are correctly suggested instead, as expected.

The patch fixed the behavior so that TO or FROM are not suggested after FOREIGN SERVER,
even when no foreign servers are defined. However, I've started to wonder if it's worth
fixing such a corner case. What do you think?

I think it's worth doing. This issue can lead to unexpected behavior
and is something users might run into. If the fix were overly complex
for a minor issue, it might not be justified. But that's not the case here.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

#27