Peter, all,

* Peter Eisentraut (pete...@gmx.net) wrote:
> Why are we not using roles and function execute privileges for this?

Alright, I've got an initial patch to do this for pg_start/stop_backup,
pg_switch_xlog, and pg_create_restore_point.  The actual backend changes
are quite small, as expected.  I'll add in the changes for the other
functions being discussed and adapt the documentation changes from
the earlier patch to make sense, but what I'd really appreciate are any
comments or thoughts regarding the changes to pg_dump (which are generic
to all of the function changes, of course).

I've added a notion of "the catalog schema" to pg_dump's internal
_namespaceinfo representation and then marked pg_catalog as being that
schema, as well as being a "dumpable" schema.  Throughout the
selectDumpable functions, I've made changes to only mark the objects in
the catalog as dumpable if they are functions.  I'm planning to look
into the extension and binary upgrade paths since I'm a bit worried
those may not work with this approach, but I wanted to get this out
there for at least an initial review as, if people feel this makes
things too ugly on the pg_dump side of things then we may want to
reconsider using the role attributes instead.

        Thanks!

                Stephen
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
new file mode 100644
index 2179bf7..36029d0
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
*************** pg_start_backup(PG_FUNCTION_ARGS)
*** 54,64 ****
  
  	backupidstr = text_to_cstring(backupid);
  
- 	if (!superuser() && !has_rolreplication(GetUserId()))
- 		ereport(ERROR,
- 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 		   errmsg("must be superuser or replication role to run a backup")));
- 
  	startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL);
  
  	PG_RETURN_LSN(startpoint);
--- 54,59 ----
*************** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,92 ****
  {
  	XLogRecPtr	stoppoint;
  
- 	if (!superuser() && !has_rolreplication(GetUserId()))
- 		ereport(ERROR,
- 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 		 (errmsg("must be superuser or replication role to run a backup"))));
- 
  	stoppoint = do_pg_stop_backup(NULL, true, NULL);
  
  	PG_RETURN_LSN(stoppoint);
--- 77,82 ----
*************** pg_switch_xlog(PG_FUNCTION_ARGS)
*** 100,110 ****
  {
  	XLogRecPtr	switchpoint;
  
- 	if (!superuser())
- 		ereport(ERROR,
- 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 			 (errmsg("must be superuser to switch transaction log files"))));
- 
  	if (RecoveryInProgress())
  		ereport(ERROR,
  				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
--- 90,95 ----
*************** pg_create_restore_point(PG_FUNCTION_ARGS
*** 129,139 ****
  	char	   *restore_name_str;
  	XLogRecPtr	restorepoint;
  
- 	if (!superuser())
- 		ereport(ERROR,
- 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 				 (errmsg("must be superuser to create a restore point"))));
- 
  	if (RecoveryInProgress())
  		ereport(ERROR,
  				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
--- 114,119 ----
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
new file mode 100644
index 2800f73..38605de
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*************** RETURNS interval
*** 897,899 ****
--- 897,907 ----
  LANGUAGE INTERNAL
  STRICT IMMUTABLE
  AS 'make_interval';
+ 
+ -- Revoke privileges for functions that should not be available to
+ -- all users.  Administrators are allowed to change this later, if
+ -- they wish.
+ REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean) FROM public;
+ REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
+ REVOKE EXECUTE ON FUNCTION pg_switch_xlog() FROM public;
+ REVOKE EXECUTE ON FUNCTION pg_create_restore_point(text) FROM public;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
new file mode 100644
index fdfb431..164608a
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** selectDumpableNamespace(NamespaceInfo *n
*** 1234,1245 ****
--- 1234,1255 ----
  	 * If specific tables are being dumped, do not dump any complete
  	 * namespaces. If specific namespaces are being dumped, dump just those
  	 * namespaces. Otherwise, dump all non-system namespaces.
+ 	 *
+ 	 * Note that we do consider dumping ACLs of functions in pg_catalog,
+ 	 * so mark that as a dumpable namespace, but further mark it as the
+ 	 * catalog namespace.
  	 */
+ 
+ 	/* Will be set when we may be dumping catalog ACLs, see below. */
+ 	nsinfo->catalog = false;
+ 
  	if (table_include_oids.head != NULL)
  		nsinfo->dobj.dump = false;
  	else if (schema_include_oids.head != NULL)
  		nsinfo->dobj.dump = simple_oid_list_member(&schema_include_oids,
  												   nsinfo->dobj.catId.oid);
+ 	else if (strncmp(nsinfo->dobj.name, "pg_catalog", 10) == 0)
+ 		nsinfo->dobj.dump = nsinfo->catalog = true;
  	else if (strncmp(nsinfo->dobj.name, "pg_", 3) == 0 ||
  			 strcmp(nsinfo->dobj.name, "information_schema") == 0)
  		nsinfo->dobj.dump = false;
*************** selectDumpableTable(TableInfo *tbinfo)
*** 1264,1276 ****
  {
  	/*
  	 * If specific tables are being dumped, dump just those tables; else, dump
! 	 * according to the parent namespace's dump flag.
  	 */
  	if (table_include_oids.head != NULL)
  		tbinfo->dobj.dump = simple_oid_list_member(&table_include_oids,
  												   tbinfo->dobj.catId.oid);
  	else
! 		tbinfo->dobj.dump = tbinfo->dobj.namespace->dobj.dump;
  
  	/*
  	 * In any case, a table can be excluded by an exclusion switch
--- 1274,1288 ----
  {
  	/*
  	 * If specific tables are being dumped, dump just those tables; else, dump
! 	 * according to the parent namespace's dump flag, except we never dump
! 	 * catalog tables.
  	 */
  	if (table_include_oids.head != NULL)
  		tbinfo->dobj.dump = simple_oid_list_member(&table_include_oids,
  												   tbinfo->dobj.catId.oid);
  	else
! 		tbinfo->dobj.dump = tbinfo->dobj.namespace->catalog ? false :
! 								tbinfo->dobj.namespace->dobj.dump;
  
  	/*
  	 * In any case, a table can be excluded by an exclusion switch
*************** selectDumpableTable(TableInfo *tbinfo)
*** 1297,1302 ****
--- 1309,1321 ----
  static void
  selectDumpableType(TypeInfo *tyinfo)
  {
+ 	/* Skip types in the catalog. */
+ 	if (tyinfo->dobj.namespace->catalog)
+ 	{
+ 		tyinfo->dobj.dump = false;
+ 		return;
+ 	}
+ 
  	/* skip complex types, except for standalone composite types */
  	if (OidIsValid(tyinfo->typrelid) &&
  		tyinfo->typrelkind != RELKIND_COMPOSITE_TYPE)
*************** selectDumpableType(TypeInfo *tyinfo)
*** 1347,1353 ****
  static void
  selectDumpableDefaultACL(DumpOptions *dopt, DefaultACLInfo *dinfo)
  {
! 	if (dinfo->dobj.namespace)
  		dinfo->dobj.dump = dinfo->dobj.namespace->dobj.dump;
  	else
  		dinfo->dobj.dump = dopt->include_everything;
--- 1366,1372 ----
  static void
  selectDumpableDefaultACL(DumpOptions *dopt, DefaultACLInfo *dinfo)
  {
! 	if (dinfo->dobj.namespace && !dinfo->dobj.namespace->catalog)
  		dinfo->dobj.dump = dinfo->dobj.namespace->dobj.dump;
  	else
  		dinfo->dobj.dump = dopt->include_everything;
*************** selectDumpableObject(DumpableObject *dob
*** 1402,1410 ****
  	/*
  	 * Default policy is to dump if parent namespace is dumpable, or always
  	 * for non-namespace-associated items.
  	 */
! 	if (dobj->namespace)
  		dobj->dump = dobj->namespace->dobj.dump;
  	else
  		dobj->dump = true;
  }
--- 1421,1433 ----
  	/*
  	 * Default policy is to dump if parent namespace is dumpable, or always
  	 * for non-namespace-associated items.
+ 	 *
+ 	 * For the catalog, however, we only consider functions currently.
  	 */
! 	if (dobj->namespace && !dobj->namespace->catalog)
  		dobj->dump = dobj->namespace->dobj.dump;
+ 	else if (dobj->namespace && dobj->namespace->catalog)
+ 		dobj->dump = dobj->objType == DO_FUNC ? true : false;
  	else
  		dobj->dump = true;
  }
*************** getFuncs(Archive *fout, DumpOptions *dop
*** 4364,4374 ****
  
  	/*
  	 * Find all user-defined functions.  Normally we can exclude functions in
! 	 * pg_catalog, which is worth doing since there are several thousand of
! 	 * 'em.  However, there are some extensions that create functions in
! 	 * pg_catalog.  In normal dumps we can still ignore those --- but in
! 	 * binary-upgrade mode, we must dump the member objects of the extension,
! 	 * so be sure to fetch any such functions.
  	 *
  	 * Also, in 9.2 and up, exclude functions that are internally dependent on
  	 * something else, since presumably those will be created as a result of
--- 4387,4397 ----
  
  	/*
  	 * Find all user-defined functions.  Normally we can exclude functions in
! 	 * pg_catalog, provided their ACLs are still the default, which is worth
! 	 * doing since there are several thousand of 'em.  However, there are
! 	 * some extensions that create functions in pg_catalog.  In normal dumps we
! 	 * can still ignore those --- but in binary-upgrade mode, we must dump the
! 	 * member objects of the extension, so be sure to fetch any such functions.
  	 *
  	 * Also, in 9.2 and up, exclude functions that are internally dependent on
  	 * something else, since presumably those will be created as a result of
*************** getFuncs(Archive *fout, DumpOptions *dop
*** 4389,4395 ****
  						  "WHERE NOT proisagg AND ("
  						  "pronamespace != "
  						  "(SELECT oid FROM pg_namespace "
! 						  "WHERE nspname = 'pg_catalog')",
  						  username_subquery);
  		if (fout->remoteVersion >= 90200)
  			appendPQExpBufferStr(query,
--- 4412,4422 ----
  						  "WHERE NOT proisagg AND ("
  						  "pronamespace != "
  						  "(SELECT oid FROM pg_namespace "
! 						  "WHERE nspname = 'pg_catalog') OR "
! 						  "(pronamespace = "
! 						  "(SELECT oid FROM pg_namespace "
! 						  "WHERE nspname = 'pg_catalog') AND "
! 						  "proacl IS NOT NULL)",
  						  username_subquery);
  		if (fout->remoteVersion >= 90200)
  			appendPQExpBufferStr(query,
*************** dumpNamespace(Archive *fout, DumpOptions
*** 8246,8252 ****
  	char	   *qnspname;
  
  	/* Skip if not to be dumped */
! 	if (!nspinfo->dobj.dump || dopt->dataOnly)
  		return;
  
  	/* don't dump dummy namespace from pre-7.3 source */
--- 8273,8279 ----
  	char	   *qnspname;
  
  	/* Skip if not to be dumped */
! 	if (!nspinfo->dobj.dump || dopt->dataOnly || nspinfo->catalog)
  		return;
  
  	/* don't dump dummy namespace from pre-7.3 source */
*************** dumpFunc(Archive *fout, DumpOptions *dop
*** 10393,10412 ****
  	if (dopt->binary_upgrade)
  		binary_upgrade_extension_member(q, &finfo->dobj, labelq->data);
  
! 	ArchiveEntry(fout, finfo->dobj.catId, finfo->dobj.dumpId,
! 				 funcsig_tag,
! 				 finfo->dobj.namespace->dobj.name,
! 				 NULL,
! 				 finfo->rolname, false,
! 				 "FUNCTION", SECTION_PRE_DATA,
! 				 q->data, delqry->data, NULL,
! 				 NULL, 0,
! 				 NULL, NULL);
  
- 	/* Dump Function Comments and Security Labels */
- 	dumpComment(fout, dopt, labelq->data,
- 				finfo->dobj.namespace->dobj.name, finfo->rolname,
- 				finfo->dobj.catId, 0, finfo->dobj.dumpId);
  	dumpSecLabel(fout, dopt, labelq->data,
  				 finfo->dobj.namespace->dobj.name, finfo->rolname,
  				 finfo->dobj.catId, 0, finfo->dobj.dumpId);
--- 10420,10449 ----
  	if (dopt->binary_upgrade)
  		binary_upgrade_extension_member(q, &finfo->dobj, labelq->data);
  
! 	/*
! 	 * Do not include the definition or comment if its a catalog function,
! 	 * except if it is the member of an extension and this is for a binary
! 	 * upgrade.
! 	 */
! 	if (!finfo->dobj.namespace->catalog ||
! 		(dopt->binary_upgrade && finfo->dobj.ext_member))
! 	{
! 		ArchiveEntry(fout, finfo->dobj.catId, finfo->dobj.dumpId,
! 					 funcsig_tag,
! 					 finfo->dobj.namespace->dobj.name,
! 					 NULL,
! 					 finfo->rolname, false,
! 					 "FUNCTION", SECTION_PRE_DATA,
! 					 q->data, delqry->data, NULL,
! 					 NULL, 0,
! 					 NULL, NULL);
! 
! 		/* Dump Function Comments and Security Labels */
! 		dumpComment(fout, dopt, labelq->data,
! 					finfo->dobj.namespace->dobj.name, finfo->rolname,
! 					finfo->dobj.catId, 0, finfo->dobj.dumpId);
! 	}
  
  	dumpSecLabel(fout, dopt, labelq->data,
  				 finfo->dobj.namespace->dobj.name, finfo->rolname,
  				 finfo->dobj.catId, 0, finfo->dobj.dumpId);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
new file mode 100644
index a9d3c10..36100a8
*** a/src/bin/pg_dump/pg_dump.h
--- b/src/bin/pg_dump/pg_dump.h
*************** typedef struct _namespaceInfo
*** 98,103 ****
--- 98,104 ----
  	DumpableObject dobj;
  	char	   *rolname;		/* name of owner, or empty string */
  	char	   *nspacl;
+ 	bool		catalog;
  } NamespaceInfo;
  
  typedef struct _extensionInfo

Attachment: signature.asc
Description: Digital signature

Reply via email to