Subject: Re: Extend ALTER DEFAULT PRIVILEGES for large objects

From: Yugo Nagata <[email-masked]>
Date: Fri, 13 Sep 2024 16:18:01 +0900

On Fri, 26 Apr 2024 17:54:06 +0900
Yugo NAGATA <[email-masked]> wrote:

> On Wed, 24 Apr 2024 16:08:39 -0500
> Nathan Bossart <[email-masked]> 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 <[email-masked]>
>From a27680b9f3031b0995fe20dd2d166e07a17ffeca Mon Sep 17 00:00:00 2001
From: Yugo Nagata <[email-masked]>
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

---

From: Laurenz Albe <[email-masked]>
Date: Wed, 22 Jan 2025 13:30:17 +0100

> 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
From 55c0ed8c09b8bd83ced894a349c01f84b7c47e82 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <[email-masked]>
Date: Wed, 22 Jan 2025 13:15:27 +0100
Subject: [PATCH v4] Extend ALTER DEFAULT PRIVILEGES for large objects

Author: Haruka Takatsuka
Author: Yugo Nagata
Discussion: https://postgr.es/m/20240424115242.236b499b2bed5b7a27f7a418%40sraoss.co.jp
---
 src/bin/psql/tab-complete.in.c                |  2 +-

index d3036c5ba9d..47c185916aa 100644
@@ -3338,7 +3338,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
 
index 89aacec4fab..3ab695892da 100644

index 02a754cc30a..9ca8a88dc91 100644
@@ -1005,6 +1005,10 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
@@ -1196,6 +1200,16 @@ SetDefaultACL(InternalDefaultACL *iacls)
 
@@ -1439,6 +1453,9 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
@@ -4250,6 +4267,10 @@ get_user_default_acl(ObjectType objtype, Oid ownerId, Oid nsp_oid)
 
index d8eb8d3deaa..b63fd57dc04 100644
@@ -2005,16 +2005,20 @@ get_object_address_defacl(List *object, bool missing_ok)
 
 	/*
@@ -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)
index 0a477a8e8a9..71a9cc134e1 100644

 

 
 
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

@@ -8141,6 +8141,7 @@ defacl_privilege_target:

@@ -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 */

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
 
@@ -2652,7 +2744,7 @@ SELECT count(*) FROM pg_shdepend
 
 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;
 
+--
2.48.1

---

From: Yugo NAGATA <[email-masked]>
Date: Thu, 23 Jan 2025 19:22:20 +0900

On Wed, 22 Jan 2025 13:30:17 +0100

> Attached is a rebased patch.

Thank you for updating the patch!

> 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 <[email-masked]>

---

From: Fujii Masao <[email-masked]>
Date: Wed, 2 Apr 2025 02:35:35 +0900

> 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

---

From: Yugo NAGATA <[email-masked]>
Date: Thu, 3 Apr 2025 23:04:18 +0900

On Wed, 2 Apr 2025 02:35:35 +0900

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

Thank you for your reviewing this patch!

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

Agreed. I attached a updated patch.

Regards,
Yugo Nagata

>From 98393b8609a97a33f1cf5ed69cb8cc28d07b25df Mon Sep 17 00:00:00 2001
From: Yugo Nagata <[email-masked]>
Date: Fri, 8 Mar 2024 17:43:43 +0900
Subject: [PATCH v5] Extend ALTER DEFAULT PRIVILEGES for large objects

 .../sgml/ref/alter_default_privileges.sgml    | 19 +++-
 14 files changed, 217 insertions(+), 13 deletions(-)

index 4558f940aaf..45ba9c5118f 100644
@@ -3360,7 +3360,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
 
index 89aacec4fab..6acd0f1df91 100644

@@ -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;

 
 	/*
 
 				if (objname)

 

 
 
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

@@ -8177,6 +8177,7 @@ defacl_privilege_target:

@@ -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
index 04c87ba8854..817cedef32c 100644
@@ -15679,6 +15679,9 @@ dumpDefaultACL(Archive *fout, const DefaultACLInfo *daclinfo)
index e038e9dc9e2..8970677ac64 100644
@@ -1222,7 +1222,9 @@ listDefaultACLs(const char *pattern)
@@ -1236,6 +1238,8 @@ listDefaultACLs(const char *pattern)
 
index 98951aef82c..c916b9299a8 100644
@@ -4457,7 +4457,7 @@ match_previous_words(int pattern_id,

index 40cf090ce61..a4af3f717a1 100644
index 5588d83e1bf..1fddb13b6ae 100644
@@ -2667,11 +2667,103 @@ SELECT has_schema_privilege('regress_priv_user2', 'testns4', 'CREATE'); -- yes
 
@@ -2682,7 +2774,7 @@ SELECT count(*) FROM pg_shdepend
 
 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;
 
+--

---

From: Fujii Masao <[email-masked]>
Date: Fri, 4 Apr 2025 00:21:27 +0900

> 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 <[email-masked]>
Co-authored-by: Yugo Nagata <[email-masked]>
Co-authored-by: Laurenz Albe <[email-masked]>
Reviewed-by: Masao Fujii <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
----------------

Regards,

---

From: Fujii Masao <[email-masked]>
Date: Fri, 4 Apr 2025 19:18:11 +0900

> 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
From 02c1811af49b5f417af71b601cb60621640a14b4 Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email-masked]>
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

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 4 Apr 2025 09:47:37 -0500

> 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

---

From: Fujii Masao <[email-masked]>
Date: Sat, 5 Apr 2025 00:10:41 +0900

> others') involves running pgindent on the entire tree periodically.

Thanks a lot!

Regards,

---

From: Yugo NAGATA <[email-masked]>
Date: Tue, 8 Apr 2025 12:28:57 +0900

On Fri, 4 Apr 2025 19:18:11 +0900

> I've pushed the patch. Thanks!

Thank you!

> 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

>From 64d5aead5ab080d40fa85f9bd51cb0a09490266f Mon Sep 17 00:00:00 2001
From: Yugo Nagata <[email-masked]>
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

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

---

From: Yugo Nagata <[email-masked]>
Date: Wed, 11 Jun 2025 11:49:37 +0900

On Tue, 8 Apr 2025 12:28:57 +0900

> foreign server names after FOREIGN SERVER.

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] https://www.postgresql.org/message-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a%40oss.nttdata.com

Best regards,
Yugo Nagata

---

From: Fujii Masao <[email-masked]>
Date: Wed, 11 Jun 2025 13:33:07 +0900

> [1] https://www.postgresql.org/message-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a%40oss.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:

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

---

From: Yugo Nagata <[email-masked]>
Date: Wed, 11 Jun 2025 13:57:37 +0900

On Wed, 11 Jun 2025 13:33:07 +0900

> 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

---

From: Yugo Nagata <[email-masked]>
Date: Wed, 11 Jun 2025 14:03:01 +0900

> But these patches go beyond that kind of issue.

Thank you for your clarification. I agreed.

Best regards,
Yugo Nagata

---

From: Fujii Masao <[email-masked]>
Date: Wed, 9 Jul 2025 20:42:42 +0900

> 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:

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,

---

From: Yugo Nagata <[email-masked]>
Date: Thu, 10 Jul 2025 10:30:41 +0900

On Wed, 9 Jul 2025 20:42:42 +0900

> 		COMPLETE_WITH_QUERY(Query_for_list_of_servers);

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

---

From: Fujii Masao <[email-masked]>
Date: Thu, 10 Jul 2025 13:23:47 +0900

On 2025/07/10 10:30, Yugo Nagata wrote:
> "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");
+       }

> 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,

---

From: Yugo Nagata <[email-masked]>
Date: Thu, 10 Jul 2025 14:11:29 +0900

On Thu, 10 Jul 2025 13:23:47 +0900

> +               if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))

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

> 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

---

From: Fujii Masao <[email-masked]>
Date: Tue, 15 Jul 2025 19:07:16 +0900

> 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,

---

From: Yugo Nagata <[email-masked]>
Date: Wed, 16 Jul 2025 16:22:31 +0900

On Tue, 15 Jul 2025 19:07:16 +0900

Thank you!

> for a minor issue, it might not be justified. But that's not the case here.

Thank you for your explanation.
It makes sense to me.

Regards,
Yugo Nagata