If Anastasia doesn't mind I'd like to send new version of the patch.
On 2019/11/28 12:29, Artur Zakirov wrote:
On 2019/11/27 13:22, Michael Paquier wrote:
Yeah, the actual take is if we want to make the frontend code more
complicated with a large set of SQL queries to check that each object
ACL is modified, which adds an additional maintenance cost in
pg_upgrade. Or if we want to keep the frontend simple and have more
backend facility to ease cross-catalog lookups for ACLs. Perhaps we
could also live with just checking after the ACLs of functions in the
short term and perhaps it covers most of the cases users would care
about.. That's tricky to conclude about and I am not sure which path
is better in the long-term, but at least it's worth discussing all
possible candidates IMO so as we make sure to not miss anything.
I checked what objects changed their signatures between master and 9.6.
I just ran pg_describe_object() for grantable object types, saved the
output into a file and diffed the outputs. It seems that only functions
and table columns changed their signatures. A list of functions is big
and here the list of columns:
table pg_attrdef column adsrc
table pg_class column relhasoids
table pg_class column relhaspkey
table pg_constraint column consrc
table pg_proc column proisagg
table pg_proc column proiswindow
table pg_proc column protransform
As a result I think in pg_upgrade we could just check functions and
columns signatures. It might simplify the patch. And if something
changes in a future we could fix pg_upgrade too.
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.
--
Artur
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index cdd8788b9e..7120ead751 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -142,6 +142,9 @@ 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);
+ if (user_opts.check)
+ 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.
@@ -181,6 +184,143 @@ check_new_cluster(void)
check_for_prepared_transactions(&new_cluster);
}
+/*
+ * check_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.
+ */
+void
+check_changed_signatures(void)
+{
+ PGconn *conn;
+ PGresult *new_res;
+ int new_ntups;
+ int i_obj_sig = 0;
+ int dbnum;
+ bool need_check;
+ FILE *script = NULL;
+ bool found_incompatible = false;
+ char output_path[MAXPGPATH];
+
+ if (!user_opts.check)
+ return;
+
+ 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),
+ "incompatible_objects_for_acl.txt");
+
+ conn = connectToServer(&new_cluster, "template1");
+ new_res = executeQueryOrDie(conn,
+ /* Get relations, languages, functions and procedures */
+ "SELECT pg_catalog.pg_describe_object(refclassid, refobjid,
refobjsubid) "
+ "FROM pg_catalog.pg_depend "
+ "WHERE deptype = 'p' AND refclassid IN "
+ " ('pg_proc'::regclass, 'pg_class'::regclass,
'pg_language'::regclass) "
+ "UNION ALL "
+ /* ...and union it with columns */
+ "SELECT pg_catalog.pg_describe_object(d.refclassid,
att.attrelid, att.attnum) "
+ "FROM pg_catalog.pg_depend d "
+ "INNER JOIN pg_catalog.pg_attribute att ON att.attrelid =
d.refobjid "
+ "WHERE d.deptype = 'p' AND d.refclassid = 'pg_class'::regclass "
+ "ORDER BY 1;");
+ new_ntups = PQntuples(new_res);
+
+ for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+ {
+ DbInfo *dbinfo = &old_cluster.dbarr.dbs[dbnum];
+ bool db_used = false;
+ int aclnum = 0,
+ new_objnum = 0;
+
+ /*
+ * For every database check system objects with non-default ACL.
+ *
+ * AclInfo array is sorted by obj_sig. 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 (new_objnum < new_ntups)
+ {
+ int ret;
+
+ ret = strcmp(PQgetvalue(new_res, new_objnum,
i_obj_sig),
+ aclinfo->obj_sig);
+ /*
+ * The new cluster doesn't have an object with
same signature,
+ * exit the loop, report below and check next
object.
+ */
+ if (ret < 0)
+ break;
+ /*
+ * The new cluster has an object with same
signatre, just exit
+ * the loop.
+ */
+ else if (ret == 0)
+ {
+ found = true;
+ new_objnum++;
+ break;
+ }
+
+ new_objnum++;
+ }
+
+ if (!found)
+ {
+ found_incompatible = 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)
+ {
+ fprintf(script, "In database: %s\n",
dbinfo->db_name);
+ db_used = true;
+ }
+ fprintf(script, " %s\n", aclinfo->obj_sig);
+ }
+ }
+ }
+
+ PQclear(new_res);
+ PQfinish(conn);
+
+ if (script)
+ fclose(script);
+
+ if (found_incompatible)
+ {
+ 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. A list of the problem
objects is in the file:\n"
+ " %s\n\n", output_path);
+ }
+ else
+ check_ok();
+}
+
void
report_clusters_compatible(void)
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index d67d3a4894..2d23d83914 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -328,6 +328,80 @@ 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 obj_sig.
+ */
+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;
+ int ntups;
+ int aclnum;
+ int i_obj_sig = 0;
+
+ res = executeQueryOrDie(conn,
+ /*
+ * Get relations, languages, functions and procedures.
+ */
+ "SELECT pg_catalog.pg_describe_object(shd.classid,
shd.objid, 0) "
+ "FROM pg_catalog.pg_shdepend shd "
+ "INNER JOIN pg_catalog.pg_depend d ON d.refclassid =
shd.classid AND "
+ " d.refobjid = shd.objid AND "
+ /* get only system objects */
+ " d.deptype = 'p' "
+ "WHERE shd.deptype = 'a' AND shd.dbid = %d AND
shd.objsubid = 0 AND "
+ " shd.classid IN "
+ " ('pg_proc'::regclass, 'pg_class'::regclass,
'pg_language'::regclass) "
+ "UNION ALL "
+ /*
+ * ...and union it with columns. We need to make an
attribute
+ * description manually because Pre-PG 11 had different
description
+ * for attributes.
+ */
+ "SELECT 'column ' || att.attname || ' of ' || "
+ " pg_catalog.pg_describe_object(shd.classid,
shd.objid, 0) "
+ "FROM pg_catalog.pg_shdepend shd "
+ "INNER JOIN pg_catalog.pg_depend d ON d.refclassid =
shd.classid AND "
+ " d.refobjid = shd.objid AND "
+ /* get only system objects */
+ " d.deptype = 'p' "
+ "INNER JOIN pg_catalog.pg_attribute att ON att.attrelid
= shd.objid AND "
+ " att.attnum = shd.objsubid "
+ "WHERE shd.deptype = 'a' AND shd.dbid = %d AND "
+ " shd.classid = 'pg_class'::regclass AND shd.objsubid
> 0 "
+ "ORDER BY 1;", dbinfo->db_oid, dbinfo->db_oid);
+ ntups = PQntuples(res);
+
+ aclinfos = (AclInfo *) pg_malloc(sizeof(AclInfo) * ntups);
+
+ for (aclnum = 0; aclnum < ntups; aclnum++)
+ {
+ AclInfo *curr = &aclinfos[aclnum];
+
+ curr->obj_sig = pg_strdup(PQgetvalue(res, aclnum,
i_obj_sig));
+ }
+
+ PQclear(res);
+ PQfinish(conn);
+
+ dbinfo->non_def_acl_arr.aclinfos = aclinfos;
+ dbinfo->non_def_acl_arr.nacls = ntups;
+ }
+}
+
+
/*
* get_db_infos()
*
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 5154a7160d..c1a41939fe 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -117,6 +117,7 @@ main(int argc, char **argv)
start_postmaster(&new_cluster, true);
check_new_cluster();
+ check_changed_signatures();
report_clusters_compatible();
pg_log(PG_REPORT,
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 729f86aa32..531d5c915b 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -149,6 +149,21 @@ typedef struct
int nrels;
} RelInfoArr;
+/*
+ * Structure to store objects information to check if it changed its signature.
+ */
+typedef struct
+{
+ char *obj_sig; /* objects signature returned by
+ *
pg_describe_object() */
+} AclInfo;
+
+typedef struct
+{
+ AclInfo *aclinfos;
+ int nacls;
+} AclInfoArr;
+
/*
* The following structure represents a relation mapping.
*/
@@ -185,6 +200,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
@@ -333,6 +350,7 @@ extern OSInfo os_info;
void output_check_banner(bool live_check);
void check_and_dump_old_cluster(bool live_check);
void check_new_cluster(void);
+void check_changed_signatures(void);
void report_clusters_compatible(void);
void issue_warnings_and_set_wal_level(void);
void output_completion_banner(char *analyze_script_file_name,
@@ -392,6 +410,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);
