14.08.2019 3:28, Stephen Frost wrote:
* Bruce Momjian (br...@momjian.us) wrote:
As much as it would be nice if the release notes covered all that, and
we updated pg_upgrade to somehow handle them, it just isn't realistic.
As we can see here, the problems often take years to show up, and even
then there were probably many other people who had the problem who never
reported it to us.
Yeah, the possible changes when you think about column-level privileges
as well really gets to be quite large..
This is why my thinking is that we should come up with additional
default roles, which aren't tied to specific catalog structures but
instead are for a more general set of capabilities which we manage and
users can either decide to use, or not. If they decide to work with the
individual functions then they have to manage the upgrade process if and
when we make changes to those functions.
Idea of having special roles looks good to me, though, I don't see
how to define what grants are needed for each role. Let's say, we
define role "backupuser" that obviously must have grants on
pg_start_backup()
and pg_stop_backup(). Should it also have access to pg_is_in_recovery()
or for example version()?
It'd be pretty neat if pg_upgrade could connect to the old and new
clusters concurrently and then perform a generic catalog comparison
between them and identify all objects which have been changed and
determine if there's any non-default ACLs or dependencies on the catalog
objects which are different between the clusters. That seems like an
awful lot of work though, and I'm not sure there's really any need,
given that we don't change the catalog for a given major version- we
could just generate the list using some queries whenever we release a
new major version and update pg_upgrade with it.
The only positive is that when pg_upgrade does fail, at least we have a
system that clearly points to the cause, but unfortunately usually at
run-time, not at --check time.
Getting it to be at check time would certainly be a great improvement.
Solving this in pg_upgrade does seem like it's probably the better
approach rather than trying to do it in pg_dump. Unfortunately, that
likely means that all we can do is have pg_upgrade point out to the user
when something will fail, with recommendations on how to address it, but
that's also something users are likely used to and willing to accept,
and puts the onus on them to consider their ACL decisions when we're
making catalog changes, and it keeps these issues out of pg_dump.
I wrote a prototype to check API and ACL compatibility (see attachment).
In the current implementation it fetches the list of system procedures
for both old and new clusters
and reports deleted functions or ACL changes during pg_upgrade check:
Performing Consistency Checks
-----------------------------
...
Checking for system functions API compatibility
dbname postgres : check procsig is equal pg_stop_backup(), procacl not
equal {anastasia=X/anastasia,backup=X/anastasia} vs {anastasia=X/anastasia}
dbname postgres : procedure pg_stop_backup(exclusive boolean, OUT lsn
pg_lsn, OUT labelfile text, OUT spcmapfile text) doesn't exist in
new_cluster
dbname postgres : procedure pg_switch_xlog() doesn't exist in new_cluster
dbname postgres : procedure pg_xlog_replay_pause() doesn't exist in
new_cluster
dbname postgres : procedure pg_xlog_replay_resume() doesn't exist in
new_cluster
...
I think it's a good first step in the right direction.
Now I have questions about implementation details:
1) How exactly should we report this incompatibility to a user?
I think it's fine to leave the warnings and also write some hint for the
user by analogy with other checks.
"Reset ACL on the problem functions to default in the old cluster to
continue"
Is it enough?
2) This approach can be extended to other catalog modifications, you
mentioned above.
I don't see what else can break pg_upgrade in the same way. However, I
don't mind
implementing more checks, while I work on this issue, if you can point
on them.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
commit 36e23da269d635ebfbc9891d37e650f8c3bfa6de
Author: Anastasia <a.lubennik...@postgrespro.ru>
Date: Mon Aug 19 19:38:51 2019 +0300
attempt to check proc signatures and ACL in pg_upgrade
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index b4cf6da..c12340a 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -90,6 +90,7 @@ check_and_dump_old_cluster(bool live_check)
get_loadable_libraries();
+ get_catalog_procedures(&old_cluster);
/*
* Check for various failure cases
@@ -148,6 +149,8 @@ check_new_cluster(void)
check_databases_are_compatible();
check_loadable_libraries();
+ get_catalog_procedures(&new_cluster);
+ check_catalog_procedures(&old_cluster, &new_cluster);
if (user_opts.transfer_mode == TRANSFER_MODE_LINK)
check_hard_link();
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index 03fd155..78c5e2c 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -275,3 +275,157 @@ check_loadable_libraries(void)
else
check_ok();
}
+
+/*
+ * qsort comparator for procedure signatures
+ */
+static int
+proc_compare_sig(const void *p1, const void *p2)
+{
+ ProcInfo *proc1 = (ProcInfo *) p1;
+ ProcInfo *proc2 = (ProcInfo *) p2;
+
+ return strcmp(proc1->procsig, proc2->procsig);
+}
+
+/*
+ * get_catalog_procedures()
+ *
+ * Fetch the signatures and ACL of cluster's system procedures.
+ *
+ * TODO We will later check that funciton's APIs are compatible
+ * and suppress dumping of removed functions where needed.
+ */
+void
+get_catalog_procedures(ClusterInfo *cluster)
+{
+ int dbnum;
+
+ /*
+ * Fetch all procedure signatures and ACL.
+ * Each procedure may have different ACL in different database.
+ */
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ DbInfo *dbinfo = &cluster->dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, dbinfo->db_name);
+ PGresult *res;
+ int num_procs;
+ int rowno;
+
+ /*
+ * Fetch procedure signatures and ACL of functions that have
+ * some non default ACL.
+ *
+ * TODO Is it necessary to handle aggregate functions?
+ * TODO Should we handle functions with default (null) ACL?
+ */
+ if (cluster->major_version >= 110000)
+ {
+ res = executeQueryOrDie(conn,
+ "select proname::text || '('"
+ " || pg_get_function_arguments(oid)::text"
+ " || ')' as funsig,"
+ " proacl from pg_proc where prokind='f'"
+ " and proacl is not null;");
+ }
+ else
+ {
+ res = executeQueryOrDie(conn,
+ "select proname::text || '('"
+ " || pg_get_function_arguments(oid)::text"
+ " || ')' as funsig,"
+ " proacl from pg_proc where proisagg = false"
+ " and proacl is not null;");
+ }
+
+ num_procs = PQntuples(res);
+ dbinfo->proc_arr.nprocs = num_procs;
+ dbinfo->proc_arr.procs = (ProcInfo *) pg_malloc(sizeof(ProcInfo) * num_procs);
+
+ for (rowno = 0; rowno < num_procs; rowno++)
+ {
+ ProcInfo *curr = &dbinfo->proc_arr.procs[rowno];
+ char *procsig = PQgetvalue(res, rowno, 0);
+ char *procacl = PQgetvalue(res, rowno, 1);
+
+ curr->procsig = pg_strdup(procsig);
+ curr->procacl = pg_strdup(procacl);
+ }
+
+ qsort((void *) dbinfo->proc_arr.procs, dbinfo->proc_arr.nprocs, sizeof(ProcInfo),
+ proc_compare_sig);
+
+ PQclear(res);
+ PQfinish(conn);
+ }
+}
+
+/*
+ * Check API changes in pg_proc between old_cluster and new_cluster.
+ * Report functions that only exist in old_cluster
+ *
+ * TODO save such functions in some list or file to show useful ereport
+ * and/or generate script to help user to revoke non-standard ACL from
+ * these functions.
+ *
+ * NOTE it's vital to call it after check_databases_are_compatible(),
+ * because we rely on correct database order
+ */
+void
+check_catalog_procedures(ClusterInfo *old_cluster, ClusterInfo *new_cluster)
+{
+ int dbnum;
+
+ prep_status("Checking for system functions API compatibility\n");
+
+ for (dbnum = 0; dbnum < old_cluster->dbarr.ndbs; dbnum++)
+ {
+ DbInfo *olddbinfo = &old_cluster->dbarr.dbs[dbnum];
+ DbInfo *newdbinfo = &new_cluster->dbarr.dbs[dbnum];
+ int i, j;
+
+ if (strcmp(olddbinfo->db_name, newdbinfo->db_name) != 0)
+ pg_log(PG_FATAL, "check_catalog_procedures failed \n");
+
+ i = j = 0;
+ while (i < olddbinfo->proc_arr.nprocs && j < newdbinfo->proc_arr.nprocs)
+ {
+ ProcInfo *oldcurr = &olddbinfo->proc_arr.procs[i];
+ ProcInfo *newcurr = &newdbinfo->proc_arr.procs[j];
+ int result = strcmp(oldcurr->procsig, newcurr->procsig);
+
+ if (result == 0)
+ {
+ /* If system function has non defaule ACL? */
+ if (strcmp(oldcurr->procacl, newcurr->procacl) != 0)
+ pg_log(PG_WARNING, "dbname %s : check procsig is equal %s, procacl not equal %s vs %s\n",
+ olddbinfo->db_name, oldcurr->procsig, oldcurr->procacl, newcurr->procacl);
+ i++;
+ j++;
+ }
+ else if (result > 0)
+ {
+// pg_log(PG_WARNING, "dbname %s : procedure %s only exist in new_cluster\n",
+// olddbinfo->db_name, newcurr->procsig );
+ j++;
+ }
+ else
+ {
+ pg_log(PG_WARNING, "dbname %s : procedure %s doesn't exist in new_cluster\n",
+ olddbinfo->db_name, oldcurr->procsig);
+ i++;
+ }
+ }
+
+ /* handle tail of old_cluster proc list */
+ while (i < olddbinfo->proc_arr.nprocs)
+ {
+ ProcInfo *oldcurr = &olddbinfo->proc_arr.procs[i];
+
+ pg_log(PG_WARNING, "dbname %s : procedure %s doesn't exist in new_cluster\n",
+ olddbinfo->db_name, oldcurr->procsig);
+ i++;
+ }
+ }
+}
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 7e5e971..5ed4f8e 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -155,6 +155,19 @@ typedef struct
int nrels;
} RelInfoArr;
+typedef struct
+{
+ char *procsig; /* procedure signature */
+ char *procacl; /* ACL */
+} ProcInfo;
+
+typedef struct
+{
+ ProcInfo *procs;
+ int nprocs;
+} ProcInfoArr;
+
+
/*
* The following structure represents a relation mapping.
*/
@@ -191,6 +204,7 @@ typedef struct
char *db_ctype;
int db_encoding;
RelInfoArr rel_arr; /* array of all user relinfos */
+ ProcInfoArr proc_arr; /* array of system procedures */
} DbInfo;
typedef struct
@@ -382,6 +396,9 @@ void check_hard_link(void);
void get_loadable_libraries(void);
void check_loadable_libraries(void);
+void get_catalog_procedures(ClusterInfo *cluster);
+void check_catalog_procedures(ClusterInfo *old_cluster,
+ ClusterInfo *new_cluster);
/* info.c */
