On 2019/12/01 23:58, Grigory Smolkin wrote:
On 11/29/19 11:07 AM, Artur Zakirov wrote:
New version of the patch differs from the previous:
- it doesn't generate script to revoke conflicting permissions (but
the patch can be fixed easily)
- generates file incompatible_objects_for_acl.txt to report which
objects changed their signatures
- uses pg_shdepend and pg_depend catalogs to get objects with custom
privileges
- uses pg_describe_object() to find objects with different signatures
Currently relations, attributes, languages, functions and procedures
are scanned. According to the documentation foreign databases,
foreign-data wrappers, foreign servers, schemas and tablespaces also
support ACLs. But some of them doesn't have entries initialized during
initdb, others like schemas and tablespaces didn't change their names.
So the patch doesn't scan such objects.
Grigory it would be great if you'll try the patch. I tested it but I
could miss something.
I`ve tested the patch on upgrade from 9.5 to master and it works now,
thank you.
Great!
But I think that 'incompatible_objects_for_acl.txt' is not a very
informative way of reporting to user the source of the problem.
There is no mentions of rolenames that holds permissions, that prevents
the upgrade, and even if they were, it is still up to user to conjure an
script to revoke all those grants, which is not a very user-friendly.
I tried to find some pattern how pg_upgrade decides to log list of
problem objects or to generate SQL script file to execute by user.
Nowadays only "Checking for large objects" and "Checking for hash
indexes" generate SQL script files and log WARNING message.
I think it should generate 'catalog_procedures.sql' script as in
previous version with all REVOKE statements, that are required to
execute for pg_upgrade to work.
I updated the patch. It generates "revoke_objects.sql" (similar to v3
patch) now and doesn't rely on --check option. It also logs still FATAL
message because it seems pg_upgrade should stop here since it fails
later if there are objects with changed identities.
The patch doesn't generate "incompatible_objects_for_acl.txt" now
because it would duplicate "revoke_objects.sql".
It now uses pg_identify_object() instead of pg_describe_object(), it is
easier to get column names with it.
--
Arthur
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index cdd8788b9e..20a3f26289 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -16,6 +16,7 @@
static void check_new_cluster_is_empty(void);
static void check_databases_are_compatible(void);
+static void check_for_changed_signatures(void);
static void check_locale_and_encoding(DbInfo *olddb, DbInfo *newdb);
static bool equivalent_locale(int category, const char *loca, const char
*locb);
static void check_is_install_user(ClusterInfo *cluster);
@@ -142,6 +143,8 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 804)
new_9_0_populate_pg_largeobject_metadata(&old_cluster, true);
+ get_non_default_acl_infos(&old_cluster);
+
/*
* While not a check option, we do this now because this is the only
time
* the old server is running.
@@ -161,6 +164,7 @@ check_new_cluster(void)
check_new_cluster_is_empty();
check_databases_are_compatible();
+ check_for_changed_signatures();
check_loadable_libraries();
@@ -443,6 +447,196 @@ check_databases_are_compatible(void)
}
}
+/*
+ * Find the location of the last dot, return NULL if not found.
+ */
+static char *
+last_dot_location(const char *identity)
+{
+ const char *p,
+ *ret = NULL;
+
+ for (p = identity; *p; p++)
+ if (*p == '.')
+ ret = p;
+ return unconstify(char *, ret);
+}
+
+/*
+ * check_for_changed_signatures()
+ *
+ * Checks that the old cluster doesn't have non-default ACL's for system
objects
+ * which had different signatures in the new cluster. Otherwise generates
+ * revoke_objects.sql.
+ */
+static void
+check_for_changed_signatures(void)
+{
+ PGconn *conn;
+ PGresult *res;
+ int ntups;
+ int i_obj_ident;
+ int dbnum;
+ bool need_check = false;
+ FILE *script = NULL;
+ bool found_changed = false;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for system objects to grant or revoke
privileges");
+
+ for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+ if (old_cluster.dbarr.dbs[dbnum].non_def_acl_arr.nacls > 0)
+ {
+ need_check = true;
+ break;
+ }
+ /*
+ * The old cluster doesn't have system objects with non-default ACL so
+ * quickly exit.
+ */
+ if (!need_check)
+ {
+ check_ok();
+ return;
+ }
+
+ snprintf(output_path, sizeof(output_path), "revoke_objects.sql");
+
+ conn = connectToServer(&new_cluster, "template1");
+ res = executeQueryOrDie(conn,
+ /* Get relations, languages, functions and procedures */
+ "SELECT obj.type, obj.identity "
+ "FROM pg_catalog.pg_depend d, "
+ "LATERAL pg_catalog.pg_identify_object("
+ " d.refclassid, d.refobjid, d.refobjsubid) obj "
+ "WHERE deptype = 'p' AND refclassid IN ('pg_proc'::regclass, "
+ " 'pg_class'::regclass,
'pg_language'::regclass) "
+ "UNION ALL "
+ /* ...and union it with columns */
+ "SELECT obj.type, obj.identity "
+ "FROM pg_catalog.pg_depend d "
+ "INNER JOIN pg_catalog.pg_attribute att ON att.attrelid =
d.refobjid, "
+ "LATERAL pg_catalog.pg_identify_object("
+ " d.refclassid, att.attrelid, att.attnum) obj "
+ "WHERE d.deptype = 'p' AND d.refclassid = 'pg_class'::regclass "
+ "ORDER BY 2;");
+ ntups = PQntuples(res);
+
+ i_obj_ident = PQfnumber(res, "identity");
+
+ for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+ {
+ DbInfo *dbinfo = &old_cluster.dbarr.dbs[dbnum];
+ bool db_used = false;
+ int aclnum = 0,
+ objnum = 0;
+
+ /*
+ * For every database check system objects with non-default ACL.
+ *
+ * AclInfo array is sorted by obj_ident. This allows us to
compare
+ * AclInfo entries with the query result above efficiently.
+ */
+ for (aclnum = 0; aclnum < dbinfo->non_def_acl_arr.nacls;
aclnum++)
+ {
+ AclInfo *aclinfo =
&dbinfo->non_def_acl_arr.aclinfos[aclnum];
+ bool found = false;
+
+ while (objnum < ntups)
+ {
+ int ret;
+
+ ret = strcmp(PQgetvalue(res, objnum,
i_obj_ident),
+ aclinfo->obj_ident);
+ /*
+ * The new cluster doesn't have an object with
same identity,
+ * exit the loop, report below and check next
object.
+ */
+ if (ret < 0)
+ break;
+ /*
+ * The new cluster has an object with same
identity, just exit
+ * the loop.
+ */
+ else if (ret == 0)
+ {
+ found = true;
+ objnum++;
+ break;
+ }
+
+ objnum++;
+ }
+
+ if (!found)
+ {
+ found_changed = true;
+ if (script == NULL && (script =
fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\":
%s\n",
+ output_path,
strerror(errno));
+ if (!db_used)
+ {
+ PQExpBufferData conn_buf;
+
+ initPQExpBuffer(&conn_buf);
+ appendPsqlMetaConnect(&conn_buf,
dbinfo->db_name);
+ fputs(conn_buf.data, script);
+ termPQExpBuffer(&conn_buf);
+
+ db_used = true;
+ }
+
+ if (strstr(aclinfo->obj_type, "column") == NULL)
+ fprintf(script, "REVOKE ALL ON %s %s
FROM %s;\n",
+ aclinfo->obj_type,
+ /* pg_identify_object()
quotes identity if necessary */
+ aclinfo->obj_ident,
+ /* role_names is
already quoted */
+ aclinfo->role_names);
+ else
+ {
+ char *pdot =
last_dot_location(aclinfo->obj_ident);
+ PQExpBufferData ident_buf;
+
+ if (pdot == NULL || *(pdot + 1) == '\0')
+ pg_fatal("invalid column
identity \"%s\"",
+
aclinfo->obj_ident);
+
+ initPQExpBuffer(&ident_buf);
+ appendBinaryPQExpBuffer(&ident_buf,
aclinfo->obj_ident,
+
pdot - aclinfo->obj_ident);
+
+ fprintf(script, "REVOKE ALL (%s) ON %s
FROM %s;\n",
+ /* pg_identify_object()
quotes identity if necessary */
+ pdot + 1,
ident_buf.data,
+ /* role_names is
already quoted */
+ aclinfo->role_names);
+ termPQExpBuffer(&ident_buf);
+ }
+ }
+ }
+ }
+
+ PQclear(res);
+ PQfinish(conn);
+
+ if (script)
+ fclose(script);
+
+ if (found_changed)
+ {
+ pg_log(PG_REPORT, "fatal\n");
+ pg_fatal("Your installation contains non-default privileges for
system objects\n"
+ "for which the API has changed. To perform
the upgrade, reset these\n"
+ "privileges to default. The file\n"
+ " %s\n"
+ "when executed by psql will revoke non-default
privileges for those objects\n\n",
+ output_path);
+ }
+ else
+ check_ok();
+}
+
/*
* create_script_for_cluster_analyze()
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index d67d3a4894..ebac412f55 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -11,6 +11,7 @@
#include "access/transam.h"
#include "catalog/pg_class_d.h"
+#include "fe_utils/string_utils.h"
#include "pg_upgrade.h"
static void create_rel_filename_map(const char *old_data, const char *new_data,
@@ -23,6 +24,7 @@ static void free_db_and_rel_infos(DbInfoArr *db_arr);
static void get_db_infos(ClusterInfo *cluster);
static void get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo);
static void free_rel_infos(RelInfoArr *rel_arr);
+static void free_acl_infos(AclInfoArr *acl_arr);
static void print_db_infos(DbInfoArr *dbinfo);
static void print_rel_infos(RelInfoArr *rel_arr);
@@ -328,6 +330,96 @@ get_db_and_rel_infos(ClusterInfo *cluster)
}
+/*
+ * get_non_default_acl_infos()
+ *
+ * Gets AclInfo array of system functions, procedures and columns information
+ * which have non-default ACL.
+ *
+ * Note: the resulting AclInfo array is assumed to be sorted by identity.
+ */
+void
+get_non_default_acl_infos(ClusterInfo *cluster)
+{
+ int dbnum;
+
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ DbInfo *dbinfo = &cluster->dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, dbinfo->db_name);
+ PGresult *res;
+ AclInfo *aclinfos;
+ AclInfo *curr;
+ int nacls = 0,
+ size_acls = 8;
+ int aclnum = 0;
+ int i_obj_type,
+ i_obj_ident,
+ i_rolname;
+
+ res = executeQueryOrDie(conn,
+ /*
+ * Get relations, attributes, languages, functions and
procedures.
+ */
+ "SELECT obj.type, obj.identity, shd.refobjid::regrole
rolename "
+ "FROM pg_catalog.pg_shdepend shd, "
+ "LATERAL pg_catalog.pg_identify_object("
+ " shd.classid, shd.objid, shd.objsubid) obj "
+ "WHERE shd.deptype = 'a' AND shd.dbid = %d "
+ " AND shd.classid IN ('pg_proc'::regclass,
'pg_class'::regclass, "
+ " 'pg_language'::regclass) "
+ /* get only system objects */
+ " AND obj.schema = 'pg_catalog' "
+ /*
+ * Sort only by identity. It should be enough to
uniquely compare
+ * objects later in check_for_changed_signatures().
+ */
+ "ORDER BY 2;", dbinfo->db_oid);
+
+ i_obj_type = PQfnumber(res, "type");
+ i_obj_ident = PQfnumber(res, "identity");
+ i_rolname = PQfnumber(res, "rolename");
+
+ aclinfos = (AclInfo *) pg_malloc(sizeof(AclInfo) * size_acls);
+
+ while (aclnum < PQntuples(res))
+ {
+ PQExpBufferData roles_buf;
+
+ if (nacls == size_acls)
+ {
+ size_acls *= 2;
+ aclinfos = (AclInfo *) pg_realloc(aclinfos,
+
sizeof(AclInfo) * size_acls);
+ }
+ curr = &aclinfos[nacls++];
+ curr->obj_type = pg_strdup(PQgetvalue(res, aclnum,
i_obj_type));
+ curr->obj_ident = pg_strdup(PQgetvalue(res, aclnum,
i_obj_ident));
+
+ initPQExpBuffer(&roles_buf);
+ /* Group all role names by objects type and identity */
+ while (aclnum < PQntuples(res) &&
+ strcmp(curr->obj_ident, PQgetvalue(res,
aclnum, i_obj_ident)) == 0)
+ {
+ if (roles_buf.len != 0)
+ appendPQExpBufferChar(&roles_buf, ',');
+
+ appendPQExpBufferStr(&roles_buf,
+ quote_identifier(PQgetvalue(res,
aclnum, i_rolname)));
+ aclnum++;
+ }
+ curr->role_names = roles_buf.data;
+ }
+
+ PQclear(res);
+ PQfinish(conn);
+
+ dbinfo->non_def_acl_arr.aclinfos = aclinfos;
+ dbinfo->non_def_acl_arr.nacls = nacls;
+ }
+}
+
+
/*
* get_db_infos()
*
@@ -595,6 +687,7 @@ free_db_and_rel_infos(DbInfoArr *db_arr)
for (dbnum = 0; dbnum < db_arr->ndbs; dbnum++)
{
free_rel_infos(&db_arr->dbs[dbnum].rel_arr);
+ free_acl_infos(&db_arr->dbs[dbnum].non_def_acl_arr);
pg_free(db_arr->dbs[dbnum].db_name);
}
pg_free(db_arr->dbs);
@@ -620,6 +713,21 @@ free_rel_infos(RelInfoArr *rel_arr)
rel_arr->nrels = 0;
}
+static void
+free_acl_infos(AclInfoArr *acl_arr)
+{
+ int aclnum;
+
+ for (aclnum = 0; aclnum < acl_arr->nacls; aclnum++)
+ {
+ pg_free(acl_arr->aclinfos[aclnum].obj_type);
+ pg_free(acl_arr->aclinfos[aclnum].obj_ident);
+ pg_free(acl_arr->aclinfos[aclnum].role_names);
+ }
+ pg_free(acl_arr->aclinfos);
+ acl_arr->nacls = 0;
+}
+
static void
print_db_infos(DbInfoArr *db_arr)
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 729f86aa32..bfc6e20de6 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -149,6 +149,23 @@ typedef struct
int nrels;
} RelInfoArr;
+/*
+ * Structure to store objects information to check if it changed its signature.
+ */
+typedef struct
+{
+ char *obj_type; /* type name */
+ char *obj_ident; /* complete object identity */
+ char *role_names; /* list of role names which have
permissions
+ * on the
object */
+} AclInfo;
+
+typedef struct
+{
+ AclInfo *aclinfos;
+ int nacls;
+} AclInfoArr;
+
/*
* The following structure represents a relation mapping.
*/
@@ -185,6 +202,8 @@ typedef struct
char *db_ctype;
int db_encoding;
RelInfoArr rel_arr; /* array of all user relinfos */
+ AclInfoArr non_def_acl_arr; /* array of objects info with
non default
+ * ACL
*/
} DbInfo;
typedef struct
@@ -392,6 +411,7 @@ FileNameMap *gen_db_file_maps(DbInfo *old_db,
DbInfo *new_db, int
*nmaps, const char *old_pgdata,
const char
*new_pgdata);
void get_db_and_rel_infos(ClusterInfo *cluster);
+void get_non_default_acl_infos(ClusterInfo *cluster);
void print_maps(FileNameMap *maps, int n,
const char *db_name);
