On 03.01.2021 14:29, Noah Misch wrote:
On Thu, Jun 11, 2020 at 07:58:43PM +0300, Anastasia Lubennikova wrote:
On 08.06.2020 19:31, Alvaro Herrera wrote:
I'm thinking what's a good way to have a test that's committable. Maybe
it would work to add a TAP test to pg_upgrade that runs initdb, does a
few GRANTS as per your attachment, then runs pg_upgrade? Currently the
pg_upgrade tests are not TAP ... we'd have to revive
https://postgr.es/m/20180126080026.gi17...@paquier.xyz
(Some progress was already made on the buildfarm front to permit this)
I would be glad to add some test, but it seems to me that the infrastructure
changes for cross-version pg_upgrade test is much more complicated task than
this modest bugfix.
Agreed.
Thank you for the review.
New version of the patch is attached, though I haven't tested it
properly yet. I am planning to do in a couple of days.
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
+static void
+check_for_changed_signatures(void)
+{
+ snprintf(output_path, sizeof(output_path), "revoke_objects.sql");
If an upgrade deleted function pg_last_wal_replay_lsn, upgrading a database in
which "REVOKE ALL ON FUNCTION pg_last_wal_replay_lsn FROM public" happened
requires a GRANT. Can you use a file name that will still make sense if
someone enhances pg_upgrade to generate such GRANT statements?
I changed the name to 'fix_system_objects_ACL.sql'. Does it look good to
you?
+ if (script == NULL && (script = fopen_priv(output_path,
"w")) == NULL)
+ pg_fatal("could not open file \"%s\":
%s\n",
+ output_path,
strerror(errno));
Use %m instead of passing sterror(errno) to %s.
Done.
+ }
+
+ /* Handle columns separately */
+ if (strstr(aclinfo->obj_type, "column") != NULL)
+ {
+ char *pdot =
last_dot_location(aclinfo->obj_ident);
I'd write strrchr(aclinfo->obj_ident, '.') instead of introducing
last_dot_location().
This assumes column names don't contain '.'; how difficult would it be to
remove that assumption? If it's difficult, a cheap approach would be to
ignore any entry containing too many dots. We're not likely to create such
column names at initdb time, but v13 does allow:
ALTER VIEW pg_locks RENAME COLUMN objid TO "a.b";
GRANT SELECT ("a.b") ON pg_locks TO backup;
After which revoke_objects.sql has:
psql:./revoke_objects.sql:9: ERROR: syntax error at or near "") ON
pg_catalog.pg_locks.""
LINE 1: REVOKE ALL (b") ON pg_catalog.pg_locks."a FROM "backup";
While that ALTER VIEW probably should have required allow_system_table_mods,
we need to assume such databases exist.
I've fixed it by using pg_identify_object_as_address(). Now we don't
have to parse obj_identity.
Overall, this patch predicts a subset of cases where pg_dump will emit a
failing GRANT or REVOKE that targets a pg_catalog object. Can you write a
code comment stating what it does and does not detect? I think it's okay to
not predict every failure, but let's record the intended coverage. Here are a
few examples I know; there may be more. The above query only finds GRANTs to
non-pinned roles. pg_dump dumps the following, but the above query doesn't
find them:
REVOKE ALL ON FUNCTION pg_last_wal_replay_lsn FROM public;
GRANT EXECUTE ON FUNCTION pg_reload_conf() TO pg_signal_backend;
The above query should test refclassid.
This function should not run queries against servers older than 9.6, because
pg_dump emits GRANT/REVOKE for pg_catalog objects only when targeting 9.6 or
later. An upgrade from 8.4 to master is failing on the above query:
Fixed.
The patch adds many lines wider than 78 columns, this being one example. In
general, break such lines. (Don't break string literal arguments of
ereport(), though.)
Fixed.
nm
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 43fc297eb6..429e4468f2 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);
@@ -151,6 +152,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.
@@ -170,6 +173,7 @@ check_new_cluster(void)
check_new_cluster_is_empty();
check_databases_are_compatible();
+ check_for_changed_signatures();
check_loadable_libraries();
@@ -328,6 +332,249 @@ check_cluster_compatibility(bool live_check)
"the old and new port numbers must be different.\n");
}
+/*
+ * check_for_changed_signatures()
+ *
+ * Check that the old cluster doesn't have non-default ACL's for system
+ * objects, which have different signatures in the new cluster,
+ * because they can cause failure during pg_restore stage of
+ * the upgrade process.
+ *
+ * If such objects exist generate fix_system_objects_ACL.sql script
+ * to adjust ACLs before upgrade.
+ *
+ * Currently supported object types are
+ * - table, view, sequence
+ * - table column, view column
+ * - procedure, function, aggregate
+ * - language
+ * - type, domain.
+ */
+static void
+check_for_changed_signatures(void)
+{
+ PGconn *conn;
+ char subquery[QUERY_ALLOC];
+ 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];
+
+ if (GET_MAJOR_VERSION(old_cluster.major_version) < 906)
+ return;
+
+ prep_status("Checking for system objects with non-default ACL");
+
+ 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 exit quickly.
+ */
+ if (!need_check)
+ {
+ check_ok();
+ return;
+ }
+
+ snprintf(output_path, sizeof(output_path), "fix_system_objects_ACL.sql");
+
+ conn = connectToServer(&new_cluster, "template1");
+
+
+ /* Gather the list of system objects' identities in a new cluster */
+
+ snprintf(subquery, sizeof(subquery),
+ /* Get system relations */
+ "SELECT 'pg_class'::regclass classid, oid objid, 0 objsubid "
+ "FROM pg_catalog.pg_class "
+ "WHERE relnamespace = 'pg_catalog'::regnamespace "
+ "UNION ALL "
+ /* Get system table columns, view columns */
+ "SELECT 'pg_class'::regclass, att.attrelid, att.attnum "
+ "FROM pg_catalog.pg_class rel "
+ "INNER JOIN pg_catalog.pg_attribute att ON rel.oid = att.attrelid "
+ "WHERE rel.relnamespace = 'pg_catalog'::regnamespace "
+ "UNION ALL "
+ /* Get system functions and procedures */
+ "SELECT 'pg_proc'::regclass, oid, 0 "
+ "FROM pg_catalog.pg_proc "
+ "WHERE pronamespace = 'pg_catalog'::regnamespace "
+ "UNION ALL "
+ /* Get system languages */
+ "SELECT 'pg_language'::regclass, oid, 0 "
+ "FROM pg_catalog.pg_language "
+ "WHERE pronamespace = 'pg_catalog'::regnamespace "
+ "UNION ALL "
+ /* Get system types and domains */
+ "SELECT 'pg_type'::regclass, oid, 0 "
+ "FROM pg_catalog.pg_type "
+ "WHERE pronamespace = 'pg_catalog'::regnamespace "
+ );
+
+ res = executeQueryOrDie(conn,
+ "SELECT ident.type, ident.identity "
+ "FROM (%s) obj, "
+ "LATERAL pg_catalog.pg_identify_object("
+ " obj.classid, obj.objid, obj.objsubid) ident "
+ /*
+ * Don't rely on database collation, since we use strcmp
+ * comparison to find non-default ACLs.
+ */
+ "ORDER BY ident.identity COLLATE \"C\";", subquery);
+
+ ntups = PQntuples(res);
+
+ i_obj_ident = PQfnumber(res, "identity");
+
+ /* For every database check system objects with non-default ACL. */
+ 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;
+
+ /*
+ *
+ * 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 report = false;
+
+ while (objnum < ntups)
+ {
+ int ret;
+
+ ret = strcmp(aclinfo->obj_ident,
+ PQgetvalue(res, objnum, i_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)
+ {
+ report = true;
+ break;
+ }
+ /*
+ * The new cluster has an object with same identity, just exit
+ * the loop.
+ */
+ else if (ret == 0)
+ {
+ objnum++;
+ break;
+ }
+ else
+ objnum++;
+ }
+
+ /* Generate REVOKE statemets for the found objects */
+ if (report)
+ {
+ found_changed = true;
+ if (script == NULL &&
+ (script = fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m\n", output_path);
+
+ /* append connect database command */
+ 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;
+ }
+
+ /* Handle table column objects */
+ if (strstr(aclinfo->obj_type, "column") != NULL)
+ {
+ char *name_pos = strstr(aclinfo->obj_ident,
+ aclinfo->obj_name);
+ PQExpBufferData ident_buf;
+
+ if (name_pos == NULL)
+ pg_fatal("invalid column identity \"%s\"",
+ aclinfo->obj_ident);
+
+ //DEBUG
+ pg_fatal("\n aclinfo->obj_ident = %s, aclinfo->obj_name = %s, name_pos = %s",
+ aclinfo->obj_ident, aclinfo->obj_name, name_pos);
+ if (*name_pos == '\"')
+ name_pos--;
+
+ initPQExpBuffer(&ident_buf);
+ appendBinaryPQExpBuffer(&ident_buf, aclinfo->obj_ident,
+ name_pos - aclinfo->obj_ident);
+
+ fprintf(script, "REVOKE ALL (%s) ON %s FROM %s;\n",
+ /* pg_identify_object() quotes identity if necessary */
+ aclinfo->obj_name, ident_buf.data,
+ /* role_names is already quoted */
+ aclinfo->role_names);
+ termPQExpBuffer(&ident_buf);
+ }
+ /*
+ * For relations except sequences we don't need to specify
+ * the object type.
+ */
+ else if (aclinfo->is_relation &&
+ strcmp(aclinfo->obj_type, "sequence") != 0)
+ fprintf(script, "REVOKE ALL ON %s FROM %s;\n",
+ /* pg_identify_object() quotes identity if necessary */
+ aclinfo->obj_ident,
+ /* role_names is already quoted */
+ aclinfo->role_names);
+ /* Other object types */
+ else
+ 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);
+ }
+ }
+ }
+
+ 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();
+}
+
/*
* check_locale_and_encoding()
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index 5d9a26cf82..2a93f4bb94 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,131 @@ get_db_and_rel_infos(ClusterInfo *cluster)
}
+/*
+ * get_non_default_acl_infos()
+ *
+ * Fill AclInfo array with information about system objects that
+ * have non-default ACL.
+ *
+ * Currently supported object types are
+ * - table, view, sequence
+ * - table column, view column
+ * - procedure, function, aggregate
+ * - language
+ * - type, domain.
+ *
+ * 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 = NULL;
+ AclInfo *curr = NULL;
+ int nacls = 0,
+ size_acls = 8;
+ int aclnum = 0;
+ int i_obj_type,
+ i_obj_ident,
+ i_obj_name,
+ i_rolname,
+ i_is_relation;
+
+ res = executeQueryOrDie(conn,
+ /*
+ * Get system objects with non-default ACLs.
+ */
+ "SELECT obj.type, obj.identity, "
+ " obj_as_address.object_names[array_length(object_names, 1)] "
+ " as name, shd.refobjid::regrole rolename, "
+ " CASE WHEN shd.classid = 'pg_class'::regclass THEN true "
+ " ELSE false "
+ " END is_relation "
+ "FROM pg_catalog.pg_shdepend shd, "
+ "LATERAL pg_catalog.pg_identify_object("
+ " shd.classid, shd.objid, shd.objsubid) obj, "
+ " pg_catalog.pg_identify_object_as_address("
+ " shd.classid, shd.objid, shd.objsubid) obj_as_address "
+ /* 'a' is for SHARED_DEPENDENCY_ACL */
+ "WHERE shd.deptype = 'a' AND shd.dbid = %d "
+ " AND shd.classid IN ('pg_proc'::regclass, "
+ " 'pg_class'::regclass, 'pg_language'::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().
+ * Don't rely on database collation, since we use strcmp
+ * comparison below.
+ */
+ "ORDER BY obj.identity COLLATE \"C\";", dbinfo->db_oid);
+
+ i_obj_type = PQfnumber(res, "type");
+ i_obj_ident = PQfnumber(res, "identity");
+ i_obj_name = PQfnumber(res, "name");
+ i_rolname = PQfnumber(res, "rolename");
+ i_is_relation = PQfnumber(res, "is_relation");
+
+ aclinfos = (AclInfo *) pg_malloc(sizeof(AclInfo) * size_acls);
+
+ while (aclnum < PQntuples(res))
+ {
+ PQExpBuffer 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));
+ curr->obj_name = pg_strdup(PQgetvalue(res, aclnum, i_obj_name));
+ curr->is_relation =
+ PQgetvalue(res, aclnum, i_is_relation)[0] == 't';
+
+ roles_buf = createPQExpBuffer();
+ initPQExpBuffer(roles_buf);
+
+ /*
+ * For each object gather string of role names mentioned in ACL
+ * in a format convenient for further use
+ * in GRANT/REVOKE statement.
+ */
+ 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 = pg_strdup(roles_buf->data);
+ destroyPQExpBuffer(roles_buf);
+ }
+
+ PQclear(res);
+ PQfinish(conn);
+
+ dbinfo->non_def_acl_arr.aclinfos = aclinfos;
+ dbinfo->non_def_acl_arr.nacls = nacls;
+ }
+}
+
+
/*
* get_db_infos()
*
@@ -384,6 +511,10 @@ get_db_infos(ClusterInfo *cluster)
dbinfos[tupnum].db_ctype = pg_strdup(PQgetvalue(res, tupnum, i_datctype));
snprintf(dbinfos[tupnum].db_tablespace, sizeof(dbinfos[tupnum].db_tablespace), "%s",
PQgetvalue(res, tupnum, i_spclocation));
+
+ /* initialize clean array */
+ dbinfos[tupnum].non_def_acl_arr.nacls = 0;
+ dbinfos[tupnum].non_def_acl_arr.aclinfos = NULL;
}
PQclear(res);
@@ -595,6 +726,9 @@ 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);
+
+ if (&db_arr->dbs[dbnum].non_def_acl_arr.nacls > 0)
+ 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 +754,23 @@ 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->aclinfos = NULL;
+ 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 919a7849fd..aca1d357cc 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -147,6 +147,29 @@ typedef struct
int nrels;
} RelInfoArr;
+/*
+ * Information about database object needed to check
+ * if its signature has changed between versions
+ * and generate REVOKE statement if necessary.
+ */
+typedef struct
+{
+ char *obj_type; /* object type */
+ char *obj_ident; /* complete object identity */
+ char *obj_name; /* object name
+ * store it separately from identity
+ * to simplify parsing */
+ bool is_relation; /* if the object is relation */
+ 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.
*/
@@ -183,6 +206,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
@@ -389,6 +414,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);
