Re: pg_upgrade fails with non-standard ACL

Greetings,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Bruce Momjian <bruce(at)momjian(dot)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