Обсуждение: 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
Вложения
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 <bruce@momjian.us> 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 +
Bruce Momjian <bruce@momjian.us> 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
Greetings, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Bruce Momjian <bruce@momjian.us> 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 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. Thanks, Stephen
Вложения
29.07.2019 18:37, Stephen Frost wrote: > Greetings, > > * Tom Lane (tgl@sss.pgh.pa.us) wrote: >> Bruce Momjian <bruce@momjian.us> 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
Вложения
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 <bruce@momjian.us> 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 +
Greetings, * Bruce Momjian (bruce@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
Вложения
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 <bruce@momjian.us> 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 +
14.08.2019 3:28, Stephen Frost wrote:
> * Bruce Momjian (bruce@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
Вложения
Greetings, * Anastasia Lubennikova (a.lubennikova@postgrespro.ru) wrote: > 14.08.2019 3:28, Stephen Frost wrote: > >* Bruce Momjian (bruce@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 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. I was thinking of, for example, column-level privileges on system relations (tables or views) where that column was later removed, for example. I do appreciate that such changes are relatively rare but they do happen... Will try to take a look at the actual patch later today. Thanks, Stephen
Вложения
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 <bruce@momjian.us> 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 +
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 <bruce@momjian.us> 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 +
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 <bruce@momjian.us> 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 +
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-fdf0a45d95d4@postgrespro.ru -- Michael
Вложения
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 <bruce@momjian.us> 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 +
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
Вложения
Greetings, * Anastasia Lubennikova (a.lubennikova@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
Вложения
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
Вложения
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
Вложения
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
Вложения
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
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
Вложения
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
Вложения
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
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
Вложения
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
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
Вложения
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
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
Вложения
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
Вложения
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
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
Вложения
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
Вложения
On 17.12.2019 11:10, Arthur Zakirov wrote:
On 2019/12/05 11:31, Michael Paquier wrote:Michael, do I understand it correctly that your concerns about pg_identify_object() relate only to the revoke sql script?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.
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've tried to test it. Script is attached.
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
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
Вложения
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 david@pgmasters.net
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
> On 25 Mar 2020, at 03:12, Artur Zakirov <zaartur@gmail.com> 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 extensionname 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
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
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
Вложения
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
Вложения
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
Вложения
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.GI17847@paquier.xyz (Some progress was already made on the buildfarm front to permit this) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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.GI17847@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
Вложения
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
Вложения
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.
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.GI17847@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
Вложения
On 21.01.2021 07:07, 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. > New version is attached along with a test script. To use it, you can simply run pg_upgrade_ACL_test.sh script in a directory that contains postrges git repository. See comments in the file. The test includes patch to rename several system objects. Renamed objects are the following: - table pg_subscription -> pg_sub - columns pg_subscription.subenabled -> subactive, - view pg_stat_subscription -> pg_stat_sub pg_stat_subscription.latest_end_lsn -> latest_end_location - function pg_stat_get_subscription -> pg_stat_get_sub - language sql -> pgsql Compared to v11, I fixed a couple of minor issues and removed language support. It seems unlikely, that we will ever change language signature, which consist of language name only. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Вложения
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
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. > >> 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. Agree. I attempted to write alternative version, but it seems too complicated. So I just added a comment about this limitation. Quoting of table column GRANT/REVOKE statements is fixed in this version. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Вложения
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. >> 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: > ... It took a while to debug new version. Now it detects and fixes both GRANT and REVOKE privileges on the catalog objects. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Вложения
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/