Re: pg_upgrade fails with non-standard ACL
This patch has been marked Waiting on Author since early March, with the thread stalled since then. I'm marking this CF entry Returned with Feedback, please feel free to resubmit it if/when a new version of the patch is available. -- Daniel Gustafsson https://vmware.com/
Re: pg_upgrade fails with non-standard ACL
On Thu, Feb 11, 2021 at 08:16:30PM +0300, Anastasia Lubennikova wrote: > On 28.01.2021 09:55, Noah Misch wrote: > >On Wed, Jan 27, 2021 at 07:32:42PM +0300, Anastasia Lubennikova wrote: > >>On 27.01.2021 14:21, Noah Misch wrote: > >>>On Mon, Jan 25, 2021 at 10:14:43PM +0300, Anastasia Lubennikova wrote: > 1) Could you please clarify, what do you mean by REVOKE failures? > > I tried following example: > > Start 9.6 cluster. > > REVOKE ALL ON function pg_switch_xlog() FROM public; > REVOKE ALL ON function pg_switch_xlog() FROM backup; > > The upgrade to the current master passes with and without patch. > It seems that current implementation of pg_upgrade doesn't take into > account > revoke ACLs. > >>>I think you can observe it by adding "revoke all on function > >>>pg_stat_get_subscription from public;" to > >>>test_add_acl_to_catalog_objects.sql > >>>and then rerunning your test script. pg_dump will reproduce that REVOKE, > >>>which would fail if postgresql.org had removed that function. That's > >>>fine, so > >>>long as a comment mentions the limitation. I've now tested exactly that. It didn't cause a test script failure, because pg_upgrade_ACL_test.sh runs "pg_upgrade --check" but does not run the final pg_upgrade (without --check). The failure happens only without --check. I've attached a modified version of your test script that reproduces the problem. It uses a different function, timenow(), so the test does not depend on test_rename_catalog_objects_v14. The attached script fails with this output: === Consult the last few lines of "pg_upgrade_dump_13325.log" for the probable cause of the failure. Failure, exiting === That log file contains: === command: "/tmp/workdir/postgresql_bin_test_new/bin/pg_dump" --host /home/nm/src/pg/backbranch/extra --port 50432 --username nm --schema-only --quote-all-identifiers --binary-upgrade --format=custom --file="pg_upgrade_dump_13325.custom" 'dbname=postgres' >> "pg_upgrade_dump_13325.log" 2>&1 command: "/tmp/workdir/postgresql_bin_test_new/bin/pg_restore" --host /home/nm/src/pg/backbranch/extra --port 50432 --username nm --clean --create --exit-on-error --verbose --dbname template1 "pg_upgrade_dump_13325.custom" >> "pg_upgrade_dump_13325.log" 2>&1 pg_restore: connecting to database for restore pg_restore: dropping DATABASE PROPERTIES postgres pg_restore: dropping DATABASE postgres pg_restore: creating DATABASE "postgres" pg_restore: connecting to new database "postgres" pg_restore: creating COMMENT "DATABASE "postgres"" pg_restore: creating DATABASE PROPERTIES "postgres" pg_restore: connecting to new database "postgres" pg_restore: creating pg_largeobject "pg_largeobject" pg_restore: creating ACL "pg_catalog.FUNCTION "timenow"()" pg_restore: while PROCESSING TOC: pg_restore: from TOC entry 3047; 0 0 ACL FUNCTION "timenow"() nm pg_restore: error: could not execute query: ERROR: function pg_catalog.timenow() does not exist Command was: REVOKE ALL ON FUNCTION "pg_catalog"."timenow"() FROM PUBLIC; === > >>In the updated patch, I implemented generation of both GRANT ALL and REVOKE > >>ALL for problematic objects. If I understand it correctly, these calls will > >>clean object's ACL completely. And I see no harm in doing this, because the > >>objects don't exist in the new cluster anyway. Here's fix_system_objects_ACL.sql with your v14 test script: === \connect postgres GRANT ALL ON function pg_catalog.pg_stat_get_subscription(pg_catalog.oid) TO "backup" ; REVOKE ALL ON function pg_catalog.pg_stat_get_subscription(pg_catalog.oid) FROM "backup" ; GRANT ALL ON pg_catalog.pg_stat_subscription TO "backup" ; REVOKE ALL ON pg_catalog.pg_stat_subscription FROM "backup" ; GRANT ALL ON pg_catalog.pg_subscription TO "backup","test" ; REVOKE ALL ON pg_catalog.pg_subscription FROM "backup","test" ; GRANT ALL (subenabled) ON pg_catalog.pg_subscription TO "backup" ; REVOKE ALL (subenabled) ON pg_catalog.pg_subscription FROM "backup" ; === Considering the REVOKE statements, those new GRANT statements have no effect. To prevent the final pg_upgrade failure, fix_system_objects_ACL.sql would need "GRANT ALL ON FUNCTION pg_catalog.pg_stat_get_subscription(pg_catalog.oid) TO PUBLIC;". Alternately, again, I don't mind if this case continues to fail, so long as a comment mentions the limitation. How would you like to proceed? One other thing: > diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c > index 43fc297eb6..f2d593f574 100644 > --- a/src/bin/pg_upgrade/check.c > +++ b/src/bin/pg_upgrade/check.c ... > +check_for_changed_signatures(void) > +{ ... > + /* > + * > + * 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++) > +
Re: pg_upgrade fails with non-standard ACL
On Wed, Jan 27, 2021 at 07:32:42PM +0300, Anastasia Lubennikova wrote: > On 27.01.2021 14:21, Noah Misch wrote: > >On Mon, Jan 25, 2021 at 10:14:43PM +0300, Anastasia Lubennikova wrote: > > > >>1) Could you please clarify, what do you mean by REVOKE failures? > >> > >>I tried following example: > >> > >>Start 9.6 cluster. > >> > >>REVOKE ALL ON function pg_switch_xlog() FROM public; > >>REVOKE ALL ON function pg_switch_xlog() FROM backup; > >> > >>The upgrade to the current master passes with and without patch. > >>It seems that current implementation of pg_upgrade doesn't take into account > >>revoke ACLs. > >I think you can observe it by adding "revoke all on function > >pg_stat_get_subscription from public;" to test_add_acl_to_catalog_objects.sql > >and then rerunning your test script. pg_dump will reproduce that REVOKE, > >which would fail if postgresql.org had removed that function. That's fine, > >so > >long as a comment mentions the limitation. > > In the updated patch, I implemented generation of both GRANT ALL and REVOKE > ALL for problematic objects. If I understand it correctly, these calls will > clean object's ACL completely. And I see no harm in doing this, because the > objects don't exist in the new cluster anyway. > > To test it I attempted to reproduce the problem, using attached > test_revoke.sql in the test. Still, pg_upgrade works fine without any > adjustments. I'd be grateful if you test it some more. test_revoke.sql has "revoke all on function pg_stat_get_subscription() from test", which does nothing. You need something that causes a REVOKE in pg_dump output. Worked example: === shell script set -x createuser test pg_dump | grep -E 'GRANT|REVOKE' psql -Xc 'revoke all on function pg_stat_get_subscription() from test;' psql -Xc 'revoke all on function pg_stat_get_subscription from test;' pg_dump | grep -E 'GRANT|REVOKE' psql -Xc 'revoke all on function pg_stat_get_subscription from public;' pg_dump | grep -E 'GRANT|REVOKE' === output + createuser test + pg_dump + grep -E 'GRANT|REVOKE' + psql -Xc 'revoke all on function pg_stat_get_subscription() from test;' ERROR: function pg_stat_get_subscription() does not exist + psql -Xc 'revoke all on function pg_stat_get_subscription from test;' REVOKE + pg_dump + grep -E 'GRANT|REVOKE' + psql -Xc 'revoke all on function pg_stat_get_subscription from public;' REVOKE + pg_dump + grep -E 'GRANT|REVOKE' REVOKE ALL ON FUNCTION pg_catalog.pg_stat_get_subscription(subid oid, OUT subid oid, OUT relid oid, OUT pid integer, OUT received_lsn pg_lsn, OUT last_msg_send_time timestamp with time zone, OUT last_msg_receipt_time timestamp with time zone, OUT latest_end_lsn pg_lsn, OUT latest_end_time timestamp with time zone) FROM PUBLIC; That REVOKE is going to fail if the upgrade target cluster lacks the function in question, and your test_rename_catalog_objects_v13 does simulate pg_stat_get_subscription not existing.
Re: pg_upgrade fails with non-standard ACL
On Mon, Jan 25, 2021 at 10:14:43PM +0300, Anastasia Lubennikova wrote: > On 24.01.2021 11:39, Noah Misch wrote: > >On Thu, Jan 21, 2021 at 01:03:58AM +0300, Anastasia Lubennikova wrote: > >>On 03.01.2021 14:29, Noah Misch wrote: > >>>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; > >I see a new comment listing object types. Please also explain the lack of > >preventing REVOKE failures (first example query above) and the limitation > >around non-pinned roles (second example). > > 1) Could you please clarify, what do you mean by REVOKE failures? > > I tried following example: > > Start 9.6 cluster. > > REVOKE ALL ON function pg_switch_xlog() FROM public; > REVOKE ALL ON function pg_switch_xlog() FROM backup; > > The upgrade to the current master passes with and without patch. > It seems that current implementation of pg_upgrade doesn't take into account > revoke ACLs. I think you can observe it by adding "revoke all on function pg_stat_get_subscription from public;" to test_add_acl_to_catalog_objects.sql and then rerunning your test script. pg_dump will reproduce that REVOKE, which would fail if postgresql.org had removed that function. That's fine, so long as a comment mentions the limitation. > 2) As for pinned roles, I think we should fix this behavior, rather than > adding a comment. Because such roles can have grants on system objects. > > In earlier versions of the patch, I gathered ACLs directly from system > catalogs: pg_proc.proacl, pg_class.relacl pg_attribute.attacl and > pg_type.typacl. > > The only downside of this approach is that it cannot be easily extended to > other object types, as we need to handle every object type separately. > I don't think it is a big deal, as we already do it in > check_for_changed_signatures() > > And also the query to gather non-standard ACLs won't look as nice as now, > because of the need to parse arrays of aclitems. > > What do you think? Hard to say for certain without seeing the code both ways. I'm not generally enthusiastic about adding pg_upgrade code to predict whether the dump will fail to restore, because such code will never be as good as just trying the restore. The patch has 413 lines of code, which is substantial. I would balk if, for example, the patch tripled in size to catch more cases.
Re: pg_upgrade fails with non-standard ACL
On 24.01.2021 11:39, Noah Misch wrote: On Thu, Jan 21, 2021 at 01:03:58AM +0300, Anastasia Lubennikova wrote: On 03.01.2021 14:29, Noah Misch wrote: 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; I see a new comment listing object types. Please also explain the lack of preventing REVOKE failures (first example query above) and the limitation around non-pinned roles (second example). 1) Could you please clarify, what do you mean by REVOKE failures? I tried following example: Start 9.6 cluster. REVOKE ALL ON function pg_switch_xlog() FROM public; REVOKE ALL ON function pg_switch_xlog() FROM backup; The upgrade to the current master passes with and without patch. It seems that current implementation of pg_upgrade doesn't take into account revoke ACLs. 2) As for pinned roles, I think we should fix this behavior, rather than adding a comment. Because such roles can have grants on system objects. In earlier versions of the patch, I gathered ACLs directly from system catalogs: pg_proc.proacl, pg_class.relacl pg_attribute.attacl and pg_type.typacl. The only downside of this approach is that it cannot be easily extended to other object types, as we need to handle every object type separately. I don't think it is a big deal, as we already do it in check_for_changed_signatures() And also the query to gather non-standard ACLs won't look as nice as now, because of the need to parse arrays of aclitems. What do you think? -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: pg_upgrade fails with non-standard ACL
On Thu, Jan 21, 2021 at 01:03:58AM +0300, Anastasia Lubennikova wrote: > On 03.01.2021 14:29, Noah Misch wrote: > >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; I see a new comment listing object types. Please also explain the lack of preventing REVOKE failures (first example query above) and the limitation around non-pinned roles (second example). > >The above query should test refclassid. Please do so. > + /* Handle table column objects */ > + if (strstr(aclinfo->obj_type, "column") != NULL) > + { > + char *name_pos = > strstr(aclinfo->obj_ident, > + > aclinfo->obj_name); > + if (*name_pos == '\"') > + name_pos--; This solves the problem affecting a column named "a.b", but it fails for a column named "pg_catalog" or "a""b". I recommend solving this by retrieving all three elements of the pg_identify_object_as_address array, then quoting each of them on the client side.
Re: pg_upgrade fails with non-standard ACL
On Thu, Jan 21, 2021 at 01:03:58AM +0300, Anastasia Lubennikova wrote: > On 03.01.2021 14:29, Noah Misch wrote: > >On Thu, Jun 11, 2020 at 07:58:43PM +0300, Anastasia Lubennikova wrote: > 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. Once that testing completes, please change the commitfest entry status to Ready for Committer. > >>+ 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? That name is fine with me. > > 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. Nice.
Re: pg_upgrade fails with non-standard ACL
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
Re: pg_upgrade fails with non-standard ACL
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. > --- 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? > + 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. > + } > + > + /* 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. > --- a/src/bin/pg_upgrade/info.c > +++ b/src/bin/pg_upgrade/info.c > +get_non_default_acl_infos(ClusterInfo *cluster) > +{ > + res = executeQueryOrDie(conn, > + /* > + * Get relations, attributes, functions and procedures. > Some system > + * objects like views are not pinned, but these type of > objects are > + * created in pg_catalog schema. > + */ > + "SELECT obj.type, obj.identity, 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 " > + /* 'a' is for SHARED_DEPENDENCY_ACL */ > + "WHERE shd.deptype = 'a' AND shd.dbid = %d " > + "AND shd.classid IN ('pg_proc'::regclass, > 'pg_class'::regclass) " > + /* get only system objects */ > + "AND obj.schema = 'pg_catalog' " 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: === Checking for large objects ok SQL command failed SELECT obj.type, obj.identity, shd.refobjid::regrole rolename, CASE WHEN shd.classid = 'pg_class'::re
Re: pg_upgrade fails with non-standard ACL
Tested this patch by running several upgrades from different major versions and different setups. ACL, that are impossible to apply, are detected and reported, so it looks good for me.
Re: pg_upgrade fails with non-standard ACL
On Thu, Jun 11, 2020 at 07:58:43PM +0300, Anastasia Lubennikova wrote: > 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. Besides, I've read the discussion and it seems that > Michael > is not going to continue this work. The main issue I recall from this patch series was the lack of enthusiasm because it would break potential users running major upgrade tests based on test.sh. At the same time, if you don't break the wheel.. > Attached v10 patch contains more fix for uninitialized variable. Thanks. Sorry for the time it takes. I'd like to get into this issue but I have not been able to dive into it seriously yet. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
On 08.06.2020 19:31, Alvaro Herrera wrote: On 2020-Jun-08, Anastasia Lubennikova wrote: In this version I rebased test patches, added some more comments, fixed memory allocation issue and also removed code that handles ACLs on languages. They require a lot of specific handling, while I doubt that their signatures, which consist of language name only, are subject to change in any future versions. 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. Besides, I've read the discussion and it seems that Michael is not going to continue this work. Attached v10 patch contains more fix for uninitialized variable. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company pg_upgrade_ACL_test.sh Description: application/shellscript commit f98c26fca3d80134a7a3cf8424d8c23891c5f2b9 Author: anastasia Date: Mon Jun 8 18:34:14 2020 +0300 pg_upgrade_ACL_check_v10.patch diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 00aef855dc..94e8528a3a 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,223 @@ 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() + * + * Check that the old cluster doesn't have non-default ACL's for system objects + * (relations, attributes, functions and procedures) which have different + * signatures in the new cluster. Otherwise generate revoke_objects.sql. + */ +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]; + + 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 + * quickly exit. + */ + if (!need_check) + { + check_ok(); + return; + } + + snprintf(output_path, sizeof(output_path), "revoke_objects.sql"); + + snprintf(subquery, sizeof(subquery), + /* Get system relations which created in pg_catalog */ + "SELECT 'pg_class'::regclass classid, oid objid, 0 objsubid " + "FROM pg_catalog.pg_class " + "WHERE relnamespace = 'pg_catalog'::regnamespace " + "UNION ALL " + /* Get system relations attributes which created in pg_catalog */ + "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 procedure which created in pg_catalog */ + "SELECT 'pg_proc'::regclass, oid, 0 " + "FROM pg_catalog.pg_proc " + "WHERE pronamespace = 'pg_catalog'::regnamespace "); + + conn = connectToServer(&new_cluster, "template1"); + 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 + * comparis
Re: pg_upgrade fails with non-standard ACL
On 2020-Jun-08, Anastasia Lubennikova wrote: > In this version I rebased test patches, added some more comments, fixed > memory allocation issue and also removed code that handles ACLs on > languages. They require a lot of specific handling, while I doubt that their > signatures, which consist of language name only, are subject to change in > any future versions. 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) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade fails with non-standard ACL
On 06.04.2020 19:40, Anastasia Lubennikova wrote: On 06.04.2020 16:49, Anastasia Lubennikova wrote: New version of the patch is attached. I fixed several issues in the check_for_changed_signatures(). Now it passes check without "test_rename_catalog_objects" and fails (generates script) with it. Test script pg_upgrade_ACL_test.sh demonstrates this. The only known issue left is the usage of pg_identify_object(), though I don't see a problem here with object types that this patch involves. As I updated the code, I will leave this patch in Need Review. One more fix for free_acl_infos(). Thanks to cfbot. One more update. In this version I rebased test patches, added some more comments, fixed memory allocation issue and also removed code that handles ACLs on languages. They require a lot of specific handling, while I doubt that their signatures, which consist of language name only, are subject to change in any future versions. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company commit c7ce324b4a213184c9b5fec18055921fc846ccad Author: anastasia Date: Mon Jun 8 18:34:14 2020 +0300 pg_upgrade_ACL_check_v9.patch diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 00aef855dc..94e8528a3a 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,223 @@ 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() + * + * Check that the old cluster doesn't have non-default ACL's for system objects + * (relations, attributes, functions and procedures) which have different + * signatures in the new cluster. Otherwise generate revoke_objects.sql. + */ +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]; + + 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 + * quickly exit. + */ + if (!need_check) + { + check_ok(); + return; + } + + snprintf(output_path, sizeof(output_path), "revoke_objects.sql"); + + snprintf(subquery, sizeof(subquery), + /* Get system relations which created in pg_catalog */ + "SELECT 'pg_class'::regclass classid, oid objid, 0 objsubid " + "FROM pg_catalog.pg_class " + "WHERE relnamespace = 'pg_catalog'::regnamespace " + "UNION ALL " + /* Get system relations attributes which created in pg_catalog */ + "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 procedure which created in pg_catalog */ + "SELECT 'pg_proc'::regclass, oid, 0 " + "FROM pg_catalog.pg_proc " + "WHERE pronamespace = 'pg_catalog'::regnamespace "); + + conn = connectToServer(&new_cluster, "template1"); + 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 (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++) + { + DbInfo *dbin
Re: pg_upgrade fails with non-standard ACL
On 06.04.2020 16:49, Anastasia Lubennikova wrote: New version of the patch is attached. I fixed several issues in the check_for_changed_signatures(). Now it passes check without "test_rename_catalog_objects" and fails (generates script) with it. Test script pg_upgrade_ACL_test.sh demonstrates this. The only known issue left is the usage of pg_identify_object(), though I don't see a problem here with object types that this patch involves. As I updated the code, I will leave this patch in Need Review. One more fix for free_acl_infos(). Thanks to cfbot. -- 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 00aef855dc..63a216c383 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,229 @@ 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; + 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]; + + 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"); + + snprintf(subquery, sizeof(subquery), + /* Get system relations which created in pg_catalog */ + "SELECT 'pg_class'::regclass classid, oid objid, 0 objsubid " + "FROM pg_catalog.pg_class " + "WHERE relnamespace = 'pg_catalog'::regnamespace " + "UNION ALL " + /* Get system relations attributes which created in pg_catalog */ + "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 procedure which created in pg_catalog */ + "SELECT 'pg_proc'::regclass, oid, 0 " + "FROM pg_catalog.pg_proc " + "WHERE pronamespace = 'pg_catalog'::regnamespace " + "UNION ALL " + /* + * Get system languages using pg_depend, since they are schema agnostic. + */ + "SELECT refclassid, refobjid, refobjsubid " + "FROM pg_catalog.pg_depend " + "WHERE deptype = 'p' AND refclassid = 'pg_language'::regclass"); + + conn = connectToServer(&new_cluster, "template1"); + 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 (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. +
Re: pg_upgrade fails with non-standard ACL
New version of the patch is attached. I fixed several issues in the check_for_changed_signatures(). Now it passes check without "test_rename_catalog_objects" and fails (generates script) with it. Test script pg_upgrade_ACL_test.sh demonstrates this. The only known issue left is the usage of pg_identify_object(), though I don't see a problem here with object types that this patch involves. As I updated the code, I will leave this patch in Need Review. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company pg_upgrade_ACL_test.sh Description: application/shellscript diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 00aef855dc..63a216c383 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,229 @@ 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; + 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]; + + 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"); + + snprintf(subquery, sizeof(subquery), + /* Get system relations which created in pg_catalog */ + "SELECT 'pg_class'::regclass classid, oid objid, 0 objsubid " + "FROM pg_catalog.pg_class " + "WHERE relnamespace = 'pg_catalog'::regnamespace " + "UNION ALL " + /* Get system relations attributes which created in pg_catalog */ + "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 procedure which created in pg_catalog */ + "SELECT 'pg_proc'::regclass, oid, 0 " + "FROM pg_catalog.pg_proc " + "WHERE pronamespace = 'pg_catalog'::regnamespace " + "UNION ALL " + /* + * Get system languages using pg_depend, since they are schema agnostic. + */ + "SELECT refclassid, refobjid, refobjsubid " + "FROM pg_catalog.pg_depend " + "WHERE deptype = 'p' AND refclassid = 'pg_language'::regclass"); + + conn = connectToServer(&new_cluster, "template1"); + 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 (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->
Re: pg_upgrade fails with non-standard ACL
On Wed, Mar 25, 2020 at 11:12:05AM +0900, Artur Zakirov wrote: > Hello David, > > On 3/25/2020 2:08 AM, David Steele wrote: > > On 12/17/19 3:10 AM, Arthur Zakirov wrote: > > > > > > I attached new version of the patch. It still uses > > > pg_identify_object(), I'm not sure about other ways to build > > > identities yet. > > > > This patch applies and builds but fails regression tests on Linux and > > Windows: > > https://travis-ci.org/postgresql-cfbot/postgresql/builds/666134656 > > https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.85292 > > Regression tests fail because cfbot applies > "test_rename_catalog_objects.patch". Regression tests pass without it. > > This patch shouldn't be applied by cfbot. I'm not sure how to do this. But > maybe it is possible to use different extension name for the test patch, not > ".patch". Yes, see Tom's message here: https://www.postgresql.org/message-id/14255.1536781029%40sss.pgh.pa.us I don't know what extensions cfbot looks for; in that case I didn't have a file extension at all. -- Justin
Re: pg_upgrade fails with non-standard ACL
> On 25 Mar 2020, at 03:12, Artur Zakirov wrote: > Regression tests fail because cfbot applies > "test_rename_catalog_objects.patch". Regression tests pass without it. > > This patch shouldn't be applied by cfbot. I'm not sure how to do this. But > maybe it is possible to use different extension name for the test patch, not > ".patch". To get around that, post a new version again without the test_ patch, that should make the cfbot pick up only the "new" patches. cheers ./daniel
Re: pg_upgrade fails with non-standard ACL
Hello David, On 3/25/2020 2:08 AM, David Steele wrote: On 12/17/19 3:10 AM, Arthur Zakirov wrote: I attached new version of the patch. It still uses pg_identify_object(), I'm not sure about other ways to build identities yet. This patch applies and builds but fails regression tests on Linux and Windows: https://travis-ci.org/postgresql-cfbot/postgresql/builds/666134656 https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.85292 Regression tests fail because cfbot applies "test_rename_catalog_objects.patch". Regression tests pass without it. This patch shouldn't be applied by cfbot. I'm not sure how to do this. But maybe it is possible to use different extension name for the test patch, not ".patch". -- Artur
Re: pg_upgrade fails with non-standard ACL
On 12/17/19 3:10 AM, Arthur Zakirov wrote: I attached new version of the patch. It still uses pg_identify_object(), I'm not sure about other ways to build identities yet. This patch applies and builds but fails regression tests on Linux and Windows: https://travis-ci.org/postgresql-cfbot/postgresql/builds/666134656 https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.85292 The CF entry has been updated to Waiting on Author. Regards, -- -David da...@pgmasters.net
Re: pg_upgrade fails with non-standard ACL
On 17.12.2019 11:10, Arthur Zakirov wrote: On 2019/12/05 11:31, Michael Paquier wrote: On Wed, Dec 04, 2019 at 06:15:52PM +0900, Arthur Zakirov wrote: Ah, I thought that pg_identify_object() gives properly quoted identity, and it could be used to make SQL script. It depends on the object type. For columns I can see in your patch that you are using a dedicated code path, but I don't think that we should assume that there is an exact one-one mapping between the object type and the grammar of GRANT/REVOKE for the object type supported because both are completely independent things and facilities. Take for example foreign data wrappers. Of course for this patch this depends if we have system object of the type which would be impacted. That's not the case of foreign data wrappers and likely it will never be, but there is no way to be sure that this won't get broken silently in the future. I attached new version of the patch. It still uses pg_identify_object(), I'm not sure about other ways to build identities yet. Michael, do I understand it correctly that your concerns about pg_identify_object() relate only to the revoke sql script? Now, I tend to agree that we should split this patch into two separate parts, to make it simpler. The first patch is to find affected objects and print warnings and the second is to generate script. I added "test_rename_catalog_objects.patch" and "test_add_acl_to_catalog_objects.sql". So testing steps are following: - initialize new source instance (e.g. pg v12) and run "test_add_acl_to_catalog_objects.sql" on it - apply "pg_upgrade_ACL_check_v6.patch" and "test_add_acl_to_catalog_objects.sql" for HEAD - initialize new target instance for HEAD - run pg_upgrade, it should create revoke_objects.sql file "test_rename_catalog_objects.patch" should be applied after "pg_upgrade_ACL_check_v6.patch". Renamed objects are the following: - table pg_subscription -> pg_sub - columns pg_subscription.subenabled -> subactive, pg_subscription.subconninfo -> subconn - view pg_stat_subscription -> pg_stat_sub - columns pg_stat_subscription.received_lsn -> received_location, pg_stat_subscription.latest_end_lsn -> latest_end_location - function pg_stat_get_subscription -> pg_stat_get_sub - language sql -> pgsql I've tried to test it. Script is attached. Described test case works. If a new cluster contains renamed objects, /pg_upgrade --check/ successfully finds them and generates a script to revoke non-default ACL. Though, without test_rename_catalog_objects.patch, /pg_upgrade --check/ still generates the same message, which is false positive. I am going to fix it and send the updated patch later this week. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company pg_upgrade_ACL_test.sh Description: application/shellscript
Re: pg_upgrade fails with non-standard ACL
Hello, On 2019/12/05 11:31, Michael Paquier wrote: On Wed, Dec 04, 2019 at 06:15:52PM +0900, Arthur Zakirov wrote: Ah, I thought that pg_identify_object() gives properly quoted identity, and it could be used to make SQL script. It depends on the object type. For columns I can see in your patch that you are using a dedicated code path, but I don't think that we should assume that there is an exact one-one mapping between the object type and the grammar of GRANT/REVOKE for the object type supported because both are completely independent things and facilities. Take for example foreign data wrappers. Of course for this patch this depends if we have system object of the type which would be impacted. That's not the case of foreign data wrappers and likely it will never be, but there is no way to be sure that this won't get broken silently in the future. I attached new version of the patch. It still uses pg_identify_object(), I'm not sure about other ways to build identities yet. It handles relations, their columns, functions, procedure and languages. There are other GRANT'able objects, like databases, foreign data wrappers and servers, schemas and tablespaces. I didn't include handling of databases, schemas and tablespaces because I don't know yet how to identify if a such object is system object other than just hard code them. They are not pinned and are not created within pg_catalog schema. Foreign data wrappers and servers are not handled too. There are no such built-in objects, so it is not possible to test them with "test_rename_catalog_objects.patch". And I'm not sure if they will be pinned (to test if it is system) if there will be such objects in the future. Sure! But I'm not sure that I understood the idea. Do you mean the patch which adds a TAP test? It can initialize two clusters, in first it renames some system objects and changes their ACLs. And finally the test runs pg_upgrade which will fail. A TAP test won't help here because the idea is to create a patch for HEAD which willingly introduces changes for system objects, where the source binaries have ACLs on object types which are broken on the target binaries with the custom patch. That's to make sure that all the logic which would get added to pu_upgrade is working correctly. Other ideas are welcome. I added "test_rename_catalog_objects.patch" and "test_add_acl_to_catalog_objects.sql". So testing steps are following: - initialize new source instance (e.g. pg v12) and run "test_add_acl_to_catalog_objects.sql" on it - apply "pg_upgrade_ACL_check_v6.patch" and "test_add_acl_to_catalog_objects.sql" for HEAD - initialize new target instance for HEAD - run pg_upgrade, it should create revoke_objects.sql file "test_rename_catalog_objects.patch" should be applied after "pg_upgrade_ACL_check_v6.patch". Renamed objects are the following: - table pg_subscription -> pg_sub - columns pg_subscription.subenabled -> subactive, pg_subscription.subconninfo -> subconn - view pg_stat_subscription -> pg_stat_sub - columns pg_stat_subscription.received_lsn -> received_location, pg_stat_subscription.latest_end_lsn -> latest_end_location - function pg_stat_get_subscription -> pg_stat_get_sub - language sql -> pgsql -- Arthur diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index cdd8788b9e..3a2004c23b 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,223 @@ 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 t
Re: pg_upgrade fails with non-standard ACL
On Wed, Dec 04, 2019 at 06:15:52PM +0900, Arthur Zakirov wrote: > On 2019/12/04 17:15, Michael Paquier wrote: >> FWIW, I am not much a fan of that part because the output generated by >> the description is most likely not compatible with the grammar >> supported. > > Ah, I thought that pg_identify_object() gives properly quoted identity, and > it could be used to make SQL script. It depends on the object type. For columns I can see in your patch that you are using a dedicated code path, but I don't think that we should assume that there is an exact one-one mapping between the object type and the grammar of GRANT/REVOKE for the object type supported because both are completely independent things and facilities. Take for example foreign data wrappers. Of course for this patch this depends if we have system object of the type which would be impacted. That's not the case of foreign data wrappers and likely it will never be, but there is no way to be sure that this won't get broken silently in the future. > Sure! But I'm not sure that I understood the idea. Do you mean the patch > which adds a TAP test? It can initialize two clusters, in first it renames > some system objects and changes their ACLs. And finally the test runs > pg_upgrade which will fail. A TAP test won't help here because the idea is to create a patch for HEAD which willingly introduces changes for system objects, where the source binaries have ACLs on object types which are broken on the target binaries with the custom patch. That's to make sure that all the logic which would get added to pu_upgrade is working correctly. Other ideas are welcome. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
On 2019/12/04 17:15, Michael Paquier wrote: On Wed, Dec 04, 2019 at 12:17:25PM +0900, Arthur Zakirov wrote: 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. (I haven't looked at the patch yet, sorry!) FWIW, I am not much a fan of that part because the output generated by the description is most likely not compatible with the grammar supported. Ah, I thought that pg_identify_object() gives properly quoted identity, and it could be used to make SQL script. In order to make the review easier, and to test for all the patterns we need to cover, I have an evil idea though. Could you write a dummy, still simple patch for HEAD which introduces backward-incompatible changes for all the object types we want to stress? Then by having ACLs on the source server which are fakely broken on the target server we can make sure that the queries we have are right, and that they report the objects we are looking for. Sure! But I'm not sure that I understood the idea. Do you mean the patch which adds a TAP test? It can initialize two clusters, in first it renames some system objects and changes their ACLs. And finally the test runs pg_upgrade which will fail. -- Arthur
Re: pg_upgrade fails with non-standard ACL
On Wed, Dec 04, 2019 at 12:17:25PM +0900, Arthur Zakirov wrote: > 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. (I haven't looked at the patch yet, sorry!) FWIW, I am not much a fan of that part because the output generated by the description is most likely not compatible with the grammar supported. In order to make the review easier, and to test for all the patterns we need to cover, I have an evil idea though. Could you write a dummy, still simple patch for HEAD which introduces backward-incompatible changes for all the object types we want to stress? Then by having ACLs on the source server which are fakely broken on the target server we can make sure that the queries we have are right, and that they report the objects we are looking for. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
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; + boolneed_check = false; + FILE *script = NULL; + boolfound_changed = false; + charoutput_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
Re: pg_upgrade fails with non-standard ACL
Hello! On 11/29/19 11:07 AM, Artur Zakirov wrote: 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. I`ve tested the patch on upgrade from 9.5 to master and it works now, thank you. 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 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. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: pg_upgrade fails with non-standard ACL
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; + boolneed_check; + FILE *script = NULL; + boolfound_incompatible = false; + charoutput_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 " + /* ..
Re: pg_upgrade fails with non-standard ACL
On 2019/11/27 13:22, Michael Paquier wrote: On Wed, Nov 27, 2019 at 11:35:14AM +0900, Artur Zakirov wrote: Other approach is similar to Anastasia's patch, which is scanning pg_proc, pg_class, pg_attribute and others to get modified ACL's and compare it with initial ACL from pg_init_privs. Next step is to find objects which names or signatures were changed using pg_describe_object() and scanning pg_depend of new cluster 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. (there is a problem here though: there are no entries for relations columns). When it comes to column ACLs, pg_shdepend stores a dependency between the column's relation and the role. Thank you for the hint. pg_shdepend could be used in a patch. -- Artur
Re: pg_upgrade fails with non-standard ACL
On Wed, Nov 27, 2019 at 11:35:14AM +0900, Artur Zakirov wrote: > I've started to implement new backend function similar to > pg_describe_object() and tried to make new version of the patch. But I'm > wondering now if it is possible to backpatch new functions to older > Postgres releases? pg_upgrade will require a presence of this function on an > older source cluster. New functions cannot be backpatched because it would require a catalog bump. > Other approach is similar to Anastasia's patch, which is scanning pg_proc, > pg_class, pg_attribute and others to get modified ACL's and compare it with > initial ACL from pg_init_privs. Next step is to find objects which names or > signatures were changed using pg_describe_object() and scanning pg_depend of > new cluster 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. > (there is a problem here though: there are no entries for > relations columns). When it comes to column ACLs, pg_shdepend stores a dependency between the column's relation and the role. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
Thank you for reviews! On 2019/11/21 17:53, Michael Paquier wrote: On Fri, Nov 15, 2019 at 11:30:02AM +0300, Grigory Smolkin wrote: On 11/9/19 5:26 AM, Michael Paquier wrote: Another question I have: do we need to care more about other extra ACLs applied to other object types? For example a subset of columns on a table with a column being renamed could be an issue. Procedure renamed in core are not that common still we did it. I think that all objects must be supported. The unfortunate part about the current approach is that it is not really scalable from the point of view of the client. What the patch does is to compare the initdb-time ACLs and the ones stored in pg_proc. In order to support all object types we would need to have more client-side logic to join properly with all the catalogs holding the ACLs of the objects to be compared. I am wondering if it would be more simple to invent a backend function which uses an input similar to pg_describe_object() with (classid, objid and objsubid) that returns the ACL of the corresponding object after looking at the catalog defined by classid. This would simplify the client part to just scan pg_init_privs... I've started to implement new backend function similar to pg_describe_object() and tried to make new version of the patch. But I'm wondering now if it is possible to backpatch new functions to older Postgres releases? pg_upgrade will require a presence of this function on an older source cluster. Other approach is similar to Anastasia's patch, which is scanning pg_proc, pg_class, pg_attribute and others to get modified ACL's and compare it with initial ACL from pg_init_privs. Next step is to find objects which names or signatures were changed using pg_describe_object() and scanning pg_depend of new cluster (there is a problem here though: there are no entries for relations columns). -- Artur
Re: pg_upgrade fails with non-standard ACL
On Thu, Nov 21, 2019 at 05:53:16PM +0900, Michael Paquier wrote: > Not arguing against the fact that it is useful, but I'd think that it > is a two-step process, where we need to understand what logic needs to > be in the backend or some frontend: > 1) Warn properly about the objects involved, where the object > description returned by pg_describe_object would be fine enough to > understand what's broken in a given database. > 2) Generate a script which may be used by the end-user. So, we have here a patch with no updates from the authors for the last two months. Anastasia, Arthur, are you still interested in this problem? Gregory has provided a review lately and has pointed out some issues. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
On Fri, Nov 15, 2019 at 11:30:02AM +0300, Grigory Smolkin wrote: > On 11/9/19 5:26 AM, Michael Paquier wrote: >> Another question I have: do we need to care more about other extra >> ACLs applied to other object types? For example a subset of columns >> on a table with a column being renamed could be an issue. Procedure >> renamed in core are not that common still we did it. > > I think that all objects must be supported. The unfortunate part about the current approach is that it is not really scalable from the point of view of the client. What the patch does is to compare the initdb-time ACLs and the ones stored in pg_proc. In order to support all object types we would need to have more client-side logic to join properly with all the catalogs holding the ACLs of the objects to be compared. I am wondering if it would be more simple to invent a backend function which uses an input similar to pg_describe_object() with (classid, objid and objsubid) that returns the ACL of the corresponding object after looking at the catalog defined by classid. This would simplify the client part to just scan pg_init_privs... >> So I think that it would be better >> for now to get to a point where we can warn about the function >> signatures involved in a given database, without the generation of the >> script with those REVOKE queries. Or would folks prefer keep that in >> the first version? My take would be to handle both separately, and to >> warn about everything so as there is no need to do pg_upgrade --check >> more than once. > > I would prefer to warn about every function (so he can more quickly assess > the situation) AND generate script. It is good to save some user time, > because he is going to need that script anyway. Not arguing against the fact that it is useful, but I'd think that it is a two-step process, where we need to understand what logic needs to be in the backend or some frontend: 1) Warn properly about the objects involved, where the object description returned by pg_describe_object would be fine enough to understand what's broken in a given database. 2) Generate a script which may be used by the end-user. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
HelLo! On 11/9/19 5:26 AM, Michael Paquier wrote: On Fri, Nov 08, 2019 at 06:03:06PM +0900, Michael Paquier wrote: I have begun looking at this one. Another question I have: do we need to care more about other extra ACLs applied to other object types? For example a subset of columns on a table with a column being renamed could be an issue. Procedure renamed in core are not that common still we did it. I think that all objects must be supported. User application may depend on them, so silently casting them to the void during upgrade may ruin someones day. But also I think, that this work should be done piecemeal, to make testing and reviewing easier, and functions are a good candidate for a first go. I think that we should keep the core part of the fix more simple by just warning about missing function signatures in the target cluster when pg_upgrade --check is used. I think that warning without any concrete details on functions involved is confusing. So I think that it would be better for now to get to a point where we can warn about the function signatures involved in a given database, without the generation of the script with those REVOKE queries. Or would folks prefer keep that in the first version? My take would be to handle both separately, and to warn about everything so as there is no need to do pg_upgrade --check more than once. I would prefer to warn about every function (so he can more quickly assess the situation) AND generate script. It is good to save some user time, because he is going to need that script anyway. I`ve tested the master patch: 1. upgrade from 9.5 and lower is broken: [gsmol@deck upgrade_workdir]$ /home/gsmol/task/13_devel/bin/pg_upgrade -b /home/gsmol/task/9.5.19/bin/ -B /home/gsmol/task/13_devel/bin/ -d /home/gsmol/task/9.5.19/data -D /tmp/upgrade Performing Consistency Checks - Checking cluster versions ok SQL command failed select proname::text || '(' || pg_get_function_arguments(pg_proc.oid)::text || ')' as funsig, array (SELECT unnest(pg_proc.proacl) EXCEPT SELECT unnest(pg_init_privs.initprivs)) from pg_proc join pg_init_privs on pg_proc.oid = pg_init_privs.objoid where pg_init_privs.classoid = 'pg_proc'::regclass::oid and pg_init_privs.privtype = 'i' and pg_proc.proisagg = false and pg_proc.proacl != pg_init_privs.initprivs; ERROR: relation "pg_init_privs" does not exist LINE 1: ...nnest(pg_init_privs.initprivs)) from pg_proc join pg_init_pr... ^ Failure, exiting 2. I think that message "Your installation contains non-default privileges for system procedures for which the API has changed." must contain versions: "Your PostgreSQL 9.5 installation contains non-default privileges for system procedures for which the API has changed in PostgreSQL 12." -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: pg_upgrade fails with non-standard ACL
On Fri, Nov 08, 2019 at 06:03:06PM +0900, Michael Paquier wrote: > I have begun looking at this one. Another question I have: do we need to care more about other extra ACLs applied to other object types? For example a subset of columns on a table with a column being renamed could be an issue. Procedure renamed in core are not that common still we did it. Here is another idea I have regarding this set of problems. We could use pg_depend on the source for system objects and join it with pg_init_privs, and then compare it with the entries of the target based on the descriptions generated by pg_describe_object(). If there is an object renamed or an unmatching signature, then we would immediately find about it, for any object types. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
On Mon, Oct 28, 2019 at 05:40:44PM +0300, Anastasia Lubennikova wrote: > I added more comments and updated the error message. > Please, feel free to fix them, if you have any suggestions. I have begun looking at this one. + /* REVOKE command must be executed in corresponding database */ + if (*new_db) + { + fprintf(*script, _("\\c %s \n"), olddbinfo->db_name); + *new_db = false; + } This will fail if the database to use includes a space? And it seems to me that log_incompatible_procedure() does not quote things properly either. +* from initial privilleges. Only check privileges set by initdb. s/privilleges/privileges/ I think that there is little point to keep get_catalog_procedures() and check_catalog_procedures() separated. Why not just using a single entry point. Wouldn't it be more simple to just use a cast as pg_proc.oid::regprocedure in the queries? Let's also put some effort in the formatting of the SQL queries here: - Schema-qualification with pg_catalog. - Format of the clauses could be better (for examples two-space indentation for clauses, etc.) I think that we should keep the core part of the fix more simple by just warning about missing function signatures in the target cluster when pg_upgrade --check is used. So I think that it would be better for now to get to a point where we can warn about the function signatures involved in a given database, without the generation of the script with those REVOKE queries. Or would folks prefer keep that in the first version? My take would be to handle both separately, and to warn about everything so as there is no need to do pg_upgrade --check more than once. I may be missing something, but it seems to me that there is no need to attach proc_arr to DbInfo or have traces of it in pg_upgrade.h as long as you keep the checks of function signatures local to the single entry point I am mentioning above. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
08.10.2019 17:08, Stephen Frost wrote: I attached the updated version. Now it prints a better error message and generates an SQL script instead of multiple warnings. The attached test script shows that. Have you tested this with extensions, where the user has changed the privileges on the extension? I'm concerned that we'll throw out warnings and tell users to go REVOKE privileges on any case where the privileges on an extension's object were changed, but that shouldn't be necessary and we should be excluding those. Good catch. Fixed in v3. I also added this check to previous test script. It passes with both v2 and v3, but v3 doesn't throw unneeded warning for extension functions. Changes to privileges on extensions should be handled just fine using the existing code, at least for upgrades of PG. Otherwise, at least imv, the code could use more comments (inside the functions, not just function-level...) and there's a few wording improvements that could be made. Again, not a full endorsement, as I just took a quick look, but it generally seems like a reasonable approach to go in and the issue with extensions was the only thing that came to mind as a potential problem. I added more comments and updated the error message. Please, feel free to fix them, if you have any suggestions. -- 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 ff7057d..623a05e 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 @@ -164,6 +165,16 @@ check_new_cluster(void) check_loadable_libraries(); + /* + * When restoring non-default ACL from old cluster we may attempt to apply + * GRANT for functions whose signatures have changed significantly. + * + * Compare function lists of old cluster and new cluster to catch + * the incompatibility early and report it to user with a nice error message + */ + get_catalog_procedures(&new_cluster); + check_catalog_procedures(&old_cluster, &new_cluster); + switch (user_opts.transfer_mode) { case TRANSFER_MODE_CLONE: diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c index ac984db..324bfd4 100644 --- a/src/bin/pg_upgrade/function.c +++ b/src/bin/pg_upgrade/function.c @@ -12,6 +12,7 @@ #include "access/transam.h" #include "catalog/pg_language_d.h" #include "pg_upgrade.h" +#include "fe_utils/string_utils.h" /* * qsort comparator for pointers to library names @@ -273,3 +274,224 @@ 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); +} + +/* + * Fetch the signatures and ACL of cluster's system procedures. + */ +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 + * non default ACL. For each procedure save only ACL that differs + * from initial privilleges. Only check privileges set by initdb. + * Objects which have initial privileges set by CREATE EXTENSION, + * will be handled by extension itself. + */ + if (cluster->major_version >= 11) + { + res = executeQueryOrDie(conn, + "select proname::text || '('" + " || pg_get_function_arguments(pg_proc.oid)::text" + " || ')' as funsig," + " array (SELECT unnest(pg_proc.proacl)" + " EXCEPT SELECT unnest(pg_init_privs.initprivs))" + " from pg_proc join pg_init_privs" + " on pg_proc.oid = pg_init_privs.objoid" + " where pg_init_privs.classoid = 'pg_proc'::regclass::oid" + " and pg_init_privs.privtype = 'i'" + " and pg_proc.prokind='f'" + " and pg_proc.proacl != pg_init_privs.initprivs;"); + } + else + { + res = executeQueryOrDie(conn, + "select proname::text || '('" + " || pg_get_function_arguments(pg_proc.oid)::text" + " || ')' as funsig," + " array (SELECT unnest(pg_proc.proacl)" + " EXCEPT SELECT unnest(pg_init_privs.initprivs))" + " from pg_proc join pg_init_privs" + " on pg_proc.oid = pg_init_privs.objoid" + " where pg_init_privs.classoid = 'pg_proc'::regclass::oid" + " and pg_init_privs.privtype = 'i'" + " and pg_proc.proisagg = false" + " and pg_proc.proacl != pg_init_pri
Re: pg_upgrade fails with non-standard ACL
Greetings, * Anastasia Lubennikova (a.lubennik...@postgrespro.ru) wrote: > Cool. It seems that everyone agrees on this patch. Thanks for working on this, I took a quick look over it and I do have some concerns. > I attached the updated version. Now it prints a better error message > and generates an SQL script instead of multiple warnings. The attached test > script shows that. Have you tested this with extensions, where the user has changed the privileges on the extension? I'm concerned that we'll throw out warnings and tell users to go REVOKE privileges on any case where the privileges on an extension's object were changed, but that shouldn't be necessary and we should be excluding those. Changes to privileges on extensions should be handled just fine using the existing code, at least for upgrades of PG. Otherwise, at least imv, the code could use more comments (inside the functions, not just function-level...) and there's a few wording improvements that could be made. Again, not a full endorsement, as I just took a quick look, but it generally seems like a reasonable approach to go in and the issue with extensions was the only thing that came to mind as a potential problem. Thanks, Stephen signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
27.09.2019 15:51, Bruce Momjian wrote: On Fri, Sep 27, 2019 at 04:22:15PM +0900, Michael Paquier wrote: On Thu, Sep 26, 2019 at 04:19:38PM -0400, Bruce Momjian wrote: On Thu, Sep 26, 2019 at 05:16:19PM -0300, Alvaro Herrera wrote: On 2019-Sep-26, Bruce Momjian wrote: Well, right now, pg_upgrade --check succeeds, but the upgrade fails. I am proposing, at a minimum, that pg_upgrade --check fails in such cases, Agreed, that should be a minimum fix. Yes. Agreed as well here. At least the latest patch proposed has the merit to track automatically functions not existing anymore from the source's version to the target's version, so patching --check offers a good compromise. Bruce, are you planning to look more at the patch posted at [1]? I did look at it. It has some TODO items listed in it still, and some C++ comments, but if everyone likes it I can apply it. Cool. It seems that everyone agrees on this patch. I attached the updated version. Now it prints a better error message and generates an SQL script instead of multiple warnings. The attached test script shows that. Patches for 10, 11, and 12 slightly differ due to merge conflicts, so I attached multiple versions. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company commit dc6d9246c6255bed2bdb2e850a21bb1fc5e0c2fc Author: Anastasia Date: Fri Oct 4 14:39:54 2019 +0300 pg_upgrade_ACL_check_v2 diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index e28b661..bfe9bbb 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 @@ -149,6 +150,16 @@ check_new_cluster(void) check_loadable_libraries(); + /* + * When restoring non-default ACL from old cluster we may attempt to apply + * GRANT for functions whose signatures have changed significantly. + * + * Compare function lists of old cluster and new cluster to catch + * the incompatibility early and report it to user with a nice error message + */ + 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 063a94f..0fce929 100644 --- a/src/bin/pg_upgrade/function.c +++ b/src/bin/pg_upgrade/function.c @@ -13,6 +13,7 @@ #include "access/transam.h" #include "catalog/pg_language.h" +#include "fe_utils/string_utils.h" /* @@ -275,3 +276,204 @@ 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); +} + +/* + * Fetch the signatures and ACL of cluster's system procedures. + */ +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. + */ + if (cluster->major_version >= 11) + { + res = executeQueryOrDie(conn, + "select proname::text || '('" + " || pg_get_function_arguments(pg_proc.oid)::text" + " || ')' as funsig," + " array (SELECT unnest(pg_proc.proacl)" + " EXCEPT SELECT unnest(pg_init_privs.initprivs))" + " from pg_proc join pg_init_privs" + " on pg_proc.oid = pg_init_privs.objoid" + " where prokind='f' and proacl != initprivs;"); + } + else + { + res = executeQueryOrDie(conn, + "select proname::text || '('" + " || pg_get_function_arguments(pg_proc.oid)::text" + " || ')' as funsig," + " array (SELECT unnest(pg_proc.proacl)" + " EXCEPT SELECT unnest(pg_init_privs.initprivs))" + " from pg_proc join pg_init_privs" + " on pg_proc.oid = pg_init_privs.objoid" + " where proisagg = false and proacl != initprivs;"); + } + + 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, + siz
Re: pg_upgrade fails with non-standard ACL
On Fri, Sep 27, 2019 at 04:22:15PM +0900, Michael Paquier wrote: > On Thu, Sep 26, 2019 at 04:19:38PM -0400, Bruce Momjian wrote: > > On Thu, Sep 26, 2019 at 05:16:19PM -0300, Alvaro Herrera wrote: > >> On 2019-Sep-26, Bruce Momjian wrote: > >>> Well, right now, pg_upgrade --check succeeds, but the upgrade fails. I > >>> am proposing, at a minimum, that pg_upgrade --check fails in such cases, > >> > >> Agreed, that should be a minimum fix. > > > > Yes. > > Agreed as well here. At least the latest patch proposed has the merit > to track automatically functions not existing anymore from the > source's version to the target's version, so patching --check offers a > good compromise. Bruce, are you planning to look more at the patch > posted at [1]? I did look at it. It has some TODO items listed in it still, and some C++ comments, but if everyone likes it I can apply it. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: pg_upgrade fails with non-standard ACL
On Thu, Sep 26, 2019 at 04:19:38PM -0400, Bruce Momjian wrote: > On Thu, Sep 26, 2019 at 05:16:19PM -0300, Alvaro Herrera wrote: >> On 2019-Sep-26, Bruce Momjian wrote: >>> Well, right now, pg_upgrade --check succeeds, but the upgrade fails. I >>> am proposing, at a minimum, that pg_upgrade --check fails in such cases, >> >> Agreed, that should be a minimum fix. > > Yes. Agreed as well here. At least the latest patch proposed has the merit to track automatically functions not existing anymore from the source's version to the target's version, so patching --check offers a good compromise. Bruce, are you planning to look more at the patch posted at [1]? [1]: https://www.postgresql.org/message-id/392ca335-068d-7bd3-0ad8-fdf0a45d9...@postgrespro.ru -- Michael signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
On Thu, Sep 26, 2019 at 05:16:19PM -0300, Alvaro Herrera wrote: > On 2019-Sep-26, Bruce Momjian wrote: > > Well, right now, pg_upgrade --check succeeds, but the upgrade fails. I > > am proposing, at a minimum, that pg_upgrade --check fails in such cases, > > Agreed, that should be a minimum fix. Yes. > > with a clear error message about how to fix it. > > So the best solution being proposed is to reset the ACL to the default? > So we would be forcing the user to propagate the ACL change manually, > rather than trying to make pg_upgrade propagate it automatically. I > suppose making pg_upgrade would be better, but I'm not sure to what > extent that is a full solution. Me neither, which is why I was proposing the minimum fix. We might not know how to fix it in all case, but maybe we can detect all cases. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: pg_upgrade fails with non-standard ACL
On 2019-Sep-26, Bruce Momjian wrote: > On Wed, Sep 11, 2019 at 06:25:38PM -0300, Álvaro Herrera wrote: > > On 2019-Aug-21, Bruce Momjian wrote: > > > > > > 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" > > > > > > Yes, I think it is good to at least throw an error during --check so > > > they don't have to find out during a live upgrade. Odds are it will > > > require manual repair. > > > > I'm not sure what you're proposing here ... are you saying that the user > > would have to modify the source cluster before pg_upgrade accepts to > > run? That sounds pretty catastrophic. > > Well, right now, pg_upgrade --check succeeds, but the upgrade fails. I > am proposing, at a minimum, that pg_upgrade --check fails in such cases, Agreed, that should be a minimum fix. > with a clear error message about how to fix it. So the best solution being proposed is to reset the ACL to the default? So we would be forcing the user to propagate the ACL change manually, rather than trying to make pg_upgrade propagate it automatically. I suppose making pg_upgrade would be better, but I'm not sure to what extent that is a full solution. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade fails with non-standard ACL
On Wed, Sep 11, 2019 at 06:25:38PM -0300, Álvaro Herrera wrote: > On 2019-Aug-21, Bruce Momjian wrote: > > > > 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" > > > > Yes, I think it is good to at least throw an error during --check so > > they don't have to find out during a live upgrade. Odds are it will > > require manual repair. > > I'm not sure what you're proposing here ... are you saying that the user > would have to modify the source cluster before pg_upgrade accepts to > run? That sounds pretty catastrophic. Well, right now, pg_upgrade --check succeeds, but the upgrade fails. I am proposing, at a minimum, that pg_upgrade --check fails in such cases, with a clear error message about how to fix it. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: pg_upgrade fails with non-standard ACL
On 2019-Aug-21, Bruce Momjian wrote: > > 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" > > Yes, I think it is good to at least throw an error during --check so > they don't have to find out during a live upgrade. Odds are it will > require manual repair. I'm not sure what you're proposing here ... are you saying that the user would have to modify the source cluster before pg_upgrade accepts to run? That sounds pretty catastrophic. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade fails with non-standard ACL
Stephen, On 2019-Aug-20, Stephen Frost wrote: > Will try to take a look at the actual patch later today. Any word on that review? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_upgrade fails with non-standard ACL
On Tue, Aug 20, 2019 at 04:38:18PM +0300, Anastasia Lubennikova wrote: > > 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" Yes, I think it is good to at least throw an error during --check so they don't have to find out during a live upgrade. Odds are it will require manual repair. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: pg_upgrade fails with non-standard ACL
Greetings, * Anastasia Lubennikova (a.lubennik...@postgrespro.ru) wrote: > 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()? pg_is_in_recovery() and version() are already allowed to be executed by public, and I don't see any particular reason to change that, so I don't believe those would need to be explicitly GRANT'd to this new role. I would think the specific set would be those listed under: https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-ADMIN-BACKUP which currently require superuser access. This isn't a new idea, btw, there was a great deal of discussion three years ago around all of this. Particularly relevant is this: https://www.postgresql.org/message-id/20160104175516.GC3685%40tamriel.snowman.net > >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? Not really sure what else we could do there..? Did you have something else in mind? We could possibly provide the specific commands to run, that seems like about the only other thing we could possibly do. > 2) This approach can be extended to other catalog modifications, you > mentioned above. > I
Re: pg_upgrade fails with non-standard ACL
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 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/b
Re: pg_upgrade fails with non-standard ACL
On Tue, Aug 13, 2019 at 08:28:12PM -0400, Stephen Frost wrote: > 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. Yeah, I think we just need to bite the bullet and create some infrastructure to help folks solve the problem. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: pg_upgrade fails with non-standard ACL
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Tue, Aug 13, 2019 at 07:04:42PM +0300, Anastasia Lubennikova wrote: > > Maybe, as a compromise, we can reset grants to default for all changed > > functions > > and also generate a script that will allow a user to preserve privileges of > > the > > old cluster by analogy with analyze_new_cluster script. > > What do you think? > > I agree pg_upgrade should work without user correction as much as > possible. However, as you can see, it can fail when user objects > reference system table objects that have changed between major releases. Right. > 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. > I think a realistic approach is to come up with a list of all the user > behaviors that can cause pg_upgrade to break (by reviewing previous > pg_upgrade bug reports), and then add code to pg_upgrade to detect them > and either fix them or report them in --check mode. In this case, we could, at least conceptually, perform a comparison between the different major versions and then check for any non-default privileges for any of the objects changed and then report on those in --check mode with a recommendation to revert to the default privileges in the old cluster before running pg_upgrade, and then apply whatever privileges are desired in the new cluster after the upgrade completes. > In summary, I am saying that the odds that patch authors, committers, > release note writers, and pg_upgrade maintainers are going to form a > consistent work flow that catches all these changes is unrealistic --- > our best bet is to create something in the pg_upgrade code to detects > this. pg_upgrade already connects to the old and new cluster, so > technically it can perform system table modification checks itself. 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. Thanks, Stephen signature.asc Description: PGP signature
Re: pg_upgrade fails with non-standard ACL
On Tue, Aug 13, 2019 at 07:04:42PM +0300, Anastasia Lubennikova wrote: > > In an ideal world, it seems like we'd make a judgement call and arrange > > to pull the privileges across when we can do so without granting the > > user privileges beyond those that were intended, and otherwise we'd > > suppress the GRANT to avoid getting an error. I'm not sure what a good > > way is to go about either figuring out a way to pull the privileges > > across, or to suppress the GRANTs when we need to (or always), would be > > though. Neither seems easy to solve in a clean way. > > > > Certainly open to suggestions. > Based on our initial bug report, I would vote against suppressing the > GRANTS, > because it leads to an unexpected failure in case a user has a special role > for > use in a third-party utility such as a backup tool, which already took care > of > internal API changes. > > Still I agree with your arguments about possibility of providing more grants > than expected. Ideally, we do not change behaviour of existing functions > that > much, but in real-world it may happen. > > Maybe, as a compromise, we can reset grants to default for all changed > functions > and also generate a script that will allow a user to preserve privileges of > the > old cluster by analogy with analyze_new_cluster script. > What do you think? I agree pg_upgrade should work without user correction as much as possible. However, as you can see, it can fail when user objects reference system table objects that have changed between major releases. 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. I think a realistic approach is to come up with a list of all the user behaviors that can cause pg_upgrade to break (by reviewing previous pg_upgrade bug reports), and then add code to pg_upgrade to detect them and either fix them or report them in --check mode. In summary, I am saying that the odds that patch authors, committers, release note writers, and pg_upgrade maintainers are going to form a consistent work flow that catches all these changes is unrealistic --- our best bet is to create something in the pg_upgrade code to detects this. pg_upgrade already connects to the old and new cluster, so technically it can perform system table modification checks itself. 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. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: pg_upgrade fails with non-standard ACL
29.07.2019 18:37, Stephen Frost wrote: Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: Bruce Momjian writes: On Thu, Jul 18, 2019 at 06:53:12PM +0300, Anastasia Lubennikova wrote: pg_upgrade from 9.6 fails if old cluster had non-standard ACL on pg_catalog functions that have changed between versions, for example pg_stop_backup(boolean). Uh, wouldn't this affect any default-installed function where the permission are modified? Is fixing only a few functions really helpful? No, it's just functions whose signatures have changed enough that a GRANT won't find them. I think the idea is that the set of potentially-affected functions is determinate. I have to say that the proposed patch seems like a complete kluge, though. For one thing we'd have to maintain the list of affected functions in each future release, and I have no faith in our remembering to do that. Well, we aren't likely to do that ourselves, no, but perhaps we could manage it with some prodding by having the buildfarm check for such cases, not unlike how library maintainers check the ABI between versions of the library they manage. Extension authors also deal with these kinds of changes routinely when writing the upgrade scripts to go between versions of their extension. I'm not convinced that this is a great approach to go down either, to be clear. When going across major versions, making people update their systems/code and re-test is typically entirely reasonable to me. Whatever we choose to do, we need to keep a list of changed functions. I don't think that it will add too much extra work to maintaining other catalog changes such as adding or renaming columns. What's more, we must mention changed functions in migration release notes. I've checked documentation [1] and found out, that function API changes are not described properly. I think it is an important omission, so I attached the patch for documentation. Not quite sure, how many users have already migrated to version 10, still, I believe it will help many others. Suppressing the GRANT also seems reasonable for the case of objects which have been renamed- clearly whatever is using those functions is going to have to be modified to deal with the new name of the function, requiring that the GRANT be re-issued doesn't seem like it's that much more to ask of users. On the other hand, properly written tools that check the version of PG and use the right function names could possibly "just work" following a major version upgrade, if the privilege was brought across to the new major version correctly. That's exactly the case. We also don't want to mistakenly GRANT users more access then they should have though- if pg_stop_backup() one day grows an optional argument to run some server-side script, I don't think we'd want to necessairly just give access to that ability to roles who, today, can execute the current pg_stop_backup() function. Of course, if we added such a capability, hopefully we would do so in a way that less privileged roles could continue to use the existing capability without having access to run such a server-side script. I also don't think that the current patch is actually sufficient to deal with all the changes we've made between the versions- what about column names on catalog tables/views that were removed, or changed/renamed..? I can't get the problem you describe here. As far as I understand, various changes of catalog tables and views are already handled correctly in pg_upgrade. In an ideal world, it seems like we'd make a judgement call and arrange to pull the privileges across when we can do so without granting the user privileges beyond those that were intended, and otherwise we'd suppress the GRANT to avoid getting an error. I'm not sure what a good way is to go about either figuring out a way to pull the privileges across, or to suppress the GRANTs when we need to (or always), would be though. Neither seems easy to solve in a clean way. Certainly open to suggestions. Based on our initial bug report, I would vote against suppressing the GRANTS, because it leads to an unexpected failure in case a user has a special role for use in a third-party utility such as a backup tool, which already took care of internal API changes. Still I agree with your arguments about possibility of providing more grants than expected. Ideally, we do not change behaviour of existing functions that much, but in real-world it may happen. Maybe, as a compromise, we can reset grants to default for all changed functions and also generate a script that will allow a user to preserve privileges of the old cluster by analogy with analyze_new_cluster script. What do you think? [1] https://www.postgresql.org/docs/10/release-10.html -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company diff --git a/doc/src/sgml/release-10.sgml b/doc/src/sgml/release-10.sgml index 7b2130e3c1..1b685b44da 100644 --- a
Re: pg_upgrade fails with non-standard ACL
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Bruce Momjian writes: > > On Thu, Jul 18, 2019 at 06:53:12PM +0300, Anastasia Lubennikova wrote: > >> pg_upgrade from 9.6 fails if old cluster had non-standard ACL > >> on pg_catalog functions that have changed between versions, > >> for example pg_stop_backup(boolean). > > > Uh, wouldn't this affect any default-installed function where the > > permission are modified? Is fixing only a few functions really helpful? > > No, it's just functions whose signatures have changed enough that > a GRANT won't find them. I think the idea is that the set of > potentially-affected functions is determinate. I have to say that > the proposed patch seems like a complete kluge, though. For one > thing we'd have to maintain the list of affected functions in each > future release, and I have no faith in our remembering to do that. Well, we aren't likely to do that ourselves, no, but perhaps we could manage it with some prodding by having the buildfarm check for such cases, not unlike how library maintainers check the ABI between versions of the library they manage. Extension authors also deal with these kinds of changes routinely when writing the upgrade scripts to go between versions of their extension. I'm not convinced that this is a great approach to go down either, to be clear. When going across major versions, making people update their systems/code and re-test is typically entirely reasonable to me. > It's also fair to question whether pg_upgrade should even try to > cope with such cases. If the function has changed signature, > it might well be that it's also changed behavior enough so that > any previously-made grants need reconsideration. (Maybe we should > just suppress the old grant rather than transferring it.) Suppressing the GRANT strikes me as pretty reasonable as an approach but wouldn't that require us to similairly track what's changed between major versions..? Unless we arrange to ignore the GRANT failing, but that seems like it would involve a fair bit of hacking around in pg_dump to have some option to ignore certain GRANTs failing. Did you have some other idea about how to suppress the old GRANT? A way to make things work for users while suppressing the GRANTS would be to add a default role for things like file-level-backup, which would be allowed to execute file-level-backup related functions, presumably even if we make changes to exactly what those function signatures are, and then encourage users to GRANT that role to the role that's allowed to log in and run the file-level backup. Obviously we wouldn't be doing that in the back-branches, but we could moving forward. > Still, this does seem like a gap in the pg_init_privs mechanism. > I wonder if Stephen has any thoughts about what ought to happen. So, in an interesting sort of way, we have a way to deal with this problem when it comes to *extensions* and I suppose that's why we haven't seen it there- namely the upgrade script, which can decide if it wants to drop an object and recreate it, or if it wants to do a create-or-replace, which would preserve the privileges (though the API has to stay the same, so that isn't exactly the same) and avoid dropping dependant objects. Unfortunately, we don't have any good way today to add an optional argument to a function while preserving the privileges on it, which would make a case like this one (and others where you'd prefer to not drop/recreate the function due to dependencies) work, for extensions. Suppressing the GRANT also seems reasonable for the case of objects which have been renamed- clearly whatever is using those functions is going to have to be modified to deal with the new name of the function, requiring that the GRANT be re-issued doesn't seem like it's that much more to ask of users. On the other hand, properly written tools that check the version of PG and use the right function names could possibly "just work" following a major version upgrade, if the privilege was brought across to the new major version correctly. We also don't want to mistakenly GRANT users more access then they should have though- if pg_stop_backup() one day grows an optional argument to run some server-side script, I don't think we'd want to necessairly just give access to that ability to roles who, today, can execute the current pg_stop_backup() function. Of course, if we added such a capability, hopefully we would do so in a way that less privileged roles could continue to use the existing capability without having access to run such a server-side script. I also don't think that the current patch is actually sufficient to deal with all the changes we've made between the versions- what about column names on catalog tables/views that were removed, or changed/renamed..? In an ideal world, it seems like we'd make a judgement call and arrange to pull the privileges across when we can do so without granting the user privileges beyond those that
Re: pg_upgrade fails with non-standard ACL
Bruce Momjian writes: > On Thu, Jul 18, 2019 at 06:53:12PM +0300, Anastasia Lubennikova wrote: >> pg_upgrade from 9.6 fails if old cluster had non-standard ACL >> on pg_catalog functions that have changed between versions, >> for example pg_stop_backup(boolean). > Uh, wouldn't this affect any default-installed function where the > permission are modified? Is fixing only a few functions really helpful? No, it's just functions whose signatures have changed enough that a GRANT won't find them. I think the idea is that the set of potentially-affected functions is determinate. I have to say that the proposed patch seems like a complete kluge, though. For one thing we'd have to maintain the list of affected functions in each future release, and I have no faith in our remembering to do that. It's also fair to question whether pg_upgrade should even try to cope with such cases. If the function has changed signature, it might well be that it's also changed behavior enough so that any previously-made grants need reconsideration. (Maybe we should just suppress the old grant rather than transferring it.) Still, this does seem like a gap in the pg_init_privs mechanism. I wonder if Stephen has any thoughts about what ought to happen. regards, tom lane
Re: pg_upgrade fails with non-standard ACL
On Thu, Jul 18, 2019 at 06:53:12PM +0300, Anastasia Lubennikova wrote: > pg_upgrade from 9.6 fails if old cluster had non-standard ACL > on pg_catalog functions that have changed between versions, > for example pg_stop_backup(boolean). > > Error: > > pg_restore: creating ACL "pg_catalog.FUNCTION "pg_stop_backup"()" > pg_restore: creating ACL "pg_catalog.FUNCTION "pg_stop_backup"("exclusive" > boolean, OUT "lsn" "pg_lsn", OUT "labelfile" "text", OUT "spcmapfile" > "text")" > pg_restore: [archiver (db)] Error while PROCESSING TOC: > pg_restore: [archiver (db)] Error from TOC entry 2169; 0 0 ACL FUNCTION > "pg_stop_backup"("exclusive" boolean, OUT "lsn" "pg_lsn", OUT "labelfile" > "text", OUT "spcmapfile" "text") anastasia > pg_restore: [archiver (db)] could not execute query: ERROR: function > pg_catalog.pg_stop_backup(boolean) does not exist > Command was: GRANT ALL ON FUNCTION > "pg_catalog"."pg_stop_backup"("exclusive" boolean, OUT "lsn" "pg_lsn", OUT > "labelfile" "text", OUT "spcmapfile" "text") TO "backup"; > > Steps to reproduce: > 1) create a database with pg9.6 > 2) create a user and change grants on pg_stop_backup(boolean): > CREATE ROLE backup WITH LOGIN; > GRANT USAGE ON SCHEMA pg_catalog TO backup; > GRANT EXECUTE ON FUNCTION pg_stop_backup() TO backup; > GRANT EXECUTE ON FUNCTION pg_stop_backup(boolean) TO backup; > 3) perform pg_upgrade to v10 (or any version above) > > The problem exists since we added to pg_dump support of ACL changes of > pg_catalog functions in commit 23f34fa4b. > > I think this is a bug since it unpredictably affects user experience, so I > propose to backpatch the fix. > Script to reproduce the problem and the patch to fix it (credit to Arthur > Zakirov) are attached. Uh, wouldn't this affect any default-installed function where the permission are modified? Is fixing only a few functions really helpful? -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
pg_upgrade fails with non-standard ACL
pg_upgrade from 9.6 fails if old cluster had non-standard ACL on pg_catalog functions that have changed between versions, for example pg_stop_backup(boolean). Error: pg_restore: creating ACL "pg_catalog.FUNCTION "pg_stop_backup"()" pg_restore: creating ACL "pg_catalog.FUNCTION "pg_stop_backup"("exclusive" boolean, OUT "lsn" "pg_lsn", OUT "labelfile" "text", OUT "spcmapfile" "text")" pg_restore: [archiver (db)] Error while PROCESSING TOC: pg_restore: [archiver (db)] Error from TOC entry 2169; 0 0 ACL FUNCTION "pg_stop_backup"("exclusive" boolean, OUT "lsn" "pg_lsn", OUT "labelfile" "text", OUT "spcmapfile" "text") anastasia pg_restore: [archiver (db)] could not execute query: ERROR: function pg_catalog.pg_stop_backup(boolean) does not exist Command was: GRANT ALL ON FUNCTION "pg_catalog"."pg_stop_backup"("exclusive" boolean, OUT "lsn" "pg_lsn", OUT "labelfile" "text", OUT "spcmapfile" "text") TO "backup"; Steps to reproduce: 1) create a database with pg9.6 2) create a user and change grants on pg_stop_backup(boolean): CREATE ROLE backup WITH LOGIN; GRANT USAGE ON SCHEMA pg_catalog TO backup; GRANT EXECUTE ON FUNCTION pg_stop_backup() TO backup; GRANT EXECUTE ON FUNCTION pg_stop_backup(boolean) TO backup; 3) perform pg_upgrade to v10 (or any version above) The problem exists since we added to pg_dump support of ACL changes of pg_catalog functions in commit 23f34fa4b. I think this is a bug since it unpredictably affects user experience, so I propose to backpatch the fix. Script to reproduce the problem and the patch to fix it (credit to Arthur Zakirov) are attached. Current patch contains a flag for pg_dump --change-old-names to enforce correct behavior. I wonder, if we can make it default behavior for pg_upgrade? -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company pg_dump_ACL_test.sh Description: application/shellscript diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h index bebec79..3c17a67 100644 --- a/src/bin/pg_dump/pg_backup.h +++ b/src/bin/pg_dump/pg_backup.h @@ -146,6 +146,7 @@ typedef struct _dumpOptions /* flags for various command-line long options */ int disable_dollar_quoting; int dump_inserts; + int change_old_names; int column_inserts; int if_exists; int no_comments; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 384b32b..4345d4e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -245,7 +245,7 @@ static void makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo, bool oids); static void buildMatViewRefreshDependencies(Archive *fout); static void getTableDataFKConstraints(void); static char *format_function_arguments(FuncInfo *finfo, char *funcargs, - bool is_agg); + bool is_agg, bool change_old_names); static char *format_function_arguments_old(Archive *fout, FuncInfo *finfo, int nallargs, char **allargtypes, @@ -360,6 +360,7 @@ main(int argc, char **argv) */ {"attribute-inserts", no_argument, &dopt.column_inserts, 1}, {"binary-upgrade", no_argument, &dopt.binary_upgrade, 1}, + {"change-old-names", no_argument, &dopt.change_old_names, 1}, {"column-inserts", no_argument, &dopt.column_inserts, 1}, {"disable-dollar-quoting", no_argument, &dopt.disable_dollar_quoting, 1}, {"disable-triggers", no_argument, &dopt.disable_triggers, 1}, @@ -11538,6 +11539,65 @@ dumpProcLang(Archive *fout, ProcLangInfo *plang) destroyPQExpBuffer(delqry); } +typedef struct FunctionMapping +{ + const char *old_name; + const char *old_args; + const char *new_name; + const char *new_args; +} FunctionMapping; + +/* + * This array is used to map old names and arguments with new values. + */ +static FunctionMapping func_mappings[] = +{ + /* Renames */ + { "pg_current_xlog_flush_location", NULL, "pg_current_wal_flush_lsn", NULL}, + { "pg_current_xlog_insert_location", NULL, "pg_current_wal_insert_lsn", NULL}, + { "pg_current_xlog_location", NULL, "pg_current_wal_lsn", NULL}, + { "pg_is_xlog_replay_paused", NULL, "pg_is_wal_replay_paused", NULL}, + { "pg_last_xlog_receive_location", NULL, "pg_last_wal_receive_lsn", NULL}, + { "pg_last_xlog_replay_location", NULL, "pg_last_wal_replay_lsn", NULL}, + { "pg_switch_xlog", NULL, "pg_switch_wal", NULL}, + { "pg_xlog_location_diff", NULL, "pg_wal_lsn_diff", NULL}, + { "pg_xlog_replay_pause", NULL, "pg_wal_replay_pause", NULL}, + { "pg_xlog_replay_resume", NULL, "pg_wal_replay_resume", NULL}, + { "pg_xlogfile_name", NULL, "pg_walfile_name", NULL}, + { "pg_xlogfile_name_offset", NULL, "pg_walfile_name_offset", NULL}, + + /* Argument changes */ + + { + "pg_create_logical_replication_slot", + "\"slot_name\" \"name\", \"plugin\" \"name\", OUT \"slot_name\" \"text\", OUT \"xlog_position\" \"pg_lsn\"", + "pg_create_logical_replication_slot", + "\"slot_name\" \"name\", \"plugin\" \"name\", \"temporary\" boolean, OUT \"slot_name\" \"text\", OUT \"lsn\" \"pg