Re: pg_upgrade fails with non-standard ACL

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