Обсуждение: ACL dump ordering broken as well for tablespaces

Hi all,

As some may have noticed, I have been looking at the ACL dump ordering
for databases, and I have noticed the same issue with tablespaces:
https://www.postgresql.org/message-id/20190522062626.GC1486@paquier.xyz

For the sake of avoiding looking at the other email, here is how to
reproduce the issue:
1) First issue those SQLs:
\! rm -rf /tmp/tbspc/
\! mkdir -p /tmp/tbspc/
CREATE ROLE a_user;
CREATE ROLE b_user WITH SUPERUSER;
CREATE ROLE c_user;
CREATE TABLESPACE poo LOCATION '/tmp/tbspc/';
SET SESSION AUTHORIZATION b_user;
REVOKE ALL ON TABLESPACE poo FROM public;
GRANT CREATE ON TABLESPACE poo TO c_user WITH GRANT OPTION;
SET SESSION AUTHORIZATION c_user;
GRANT CREATE ON TABLESPACE poo TO a_user
2) Use pg_dumpall -g, where you would notice the following set of
GRANT queries:
CREATE TABLESPACE poo OWNER postgres LOCATION '/tmp/tbspc';
SET SESSION AUTHORIZATION c_user;
GRANT ALL ON TABLESPACE poo TO a_user;
RESET SESSION AUTHORIZATION;
GRANT ALL ON TABLESPACE poo TO c_user WITH GRANT OPTION;
3) Trying to restore results in a failure for the first GRANT query,
as the second one has not set yet the authorizations for c_user.

Attached is a patch to fix that, so as pg_dumpall does not complain
when piling up GRANT commands using WITH GRANT OPTION.  Are there any
complains to apply that down to 9.6?

When applying the patch, the set of GRANT queries is reordered:
 CREATE TABLESPACE poo OWNER postgres LOCATION '/tmp/tbspc';
+GRANT ALL ON TABLESPACE poo TO c_user WITH GRANT OPTION;
 SET SESSION AUTHORIZATION c_user;
 GRANT ALL ON TABLESPACE poo TO a_user;
 RESET SESSION AUTHORIZATION;
-GRANT ALL ON TABLESPACE poo TO c_user WITH GRANT OPTION;

As the problem is kind of different than the database case, I wanted
to spawn anyway a new thread, but I got a bonus question: what would
it take to support pg_init_privs for databases and tablespaces?  If we
could get that to work, then all the ACL-related queries built for all
objects could make use of buildACLQueries(), which would avoid extra
diffs in the dump code for dbs and tbspaces.

Thoughts?
--
Michael

On 5/22/19, 12:16 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Attached is a patch to fix that, so as pg_dumpall does not complain
> when piling up GRANT commands using WITH GRANT OPTION.  Are there any
> complains to apply that down to 9.6?

The patch looks good to me.

> As the problem is kind of different than the database case, I wanted
> to spawn anyway a new thread, but I got a bonus question: what would
> it take to support pg_init_privs for databases and tablespaces?  If we
> could get that to work, then all the ACL-related queries built for all
> objects could make use of buildACLQueries(), which would avoid extra
> diffs in the dump code for dbs and tbspaces.

A bit of digging led me to the commit that removed databases and
tablespaces from pg_init_privs [0] and to a related thread [1].  IIUC
the problem is that using pg_init_privs for databases is complicated
by the ability to drop and recreate the template databases.

Nathan

[0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47f5bb9f539a7fff089724b1cbacc31613031895
[1] https://www.postgresql.org/message-id/9f25cb66-df67-8d81-ed6a-d18692a03410%402ndquadrant.com

On Wed, May 22, 2019 at 06:35:31PM +0000, Bossart, Nathan wrote:
> The patch looks good to me.

Thanks for double-checking.  I have applied and back-patched.  The good
thing here is that there were zero conflicts.

> A bit of digging led me to the commit that removed databases and
> tablespaces from pg_init_privs [0] and to a related thread [1].  IIUC
> the problem is that using pg_init_privs for databases is complicated
> by the ability to drop and recreate the template databases.

I don't quite get this argument.  If a user is willing to drop
template1, then it is logic to also drop its initial privilege entries
and recreate new ones from scratch.  I think that this deserves a
closer lookup.  For tablespaces, we are limited by the ability of not
sharing pg_init_privs?
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Wed, May 22, 2019 at 06:35:31PM +0000, Bossart, Nathan wrote:
> > A bit of digging led me to the commit that removed databases and
> > tablespaces from pg_init_privs [0] and to a related thread [1].  IIUC
> > the problem is that using pg_init_privs for databases is complicated
> > by the ability to drop and recreate the template databases.
>
> I don't quite get this argument.  If a user is willing to drop
> template1, then it is logic to also drop its initial privilege entries
> and recreate new ones from scratch.  I think that this deserves a
> closer lookup.  For tablespaces, we are limited by the ability of not
> sharing pg_init_privs?

Why do you feel that's the case?  The point of pg_init_privs is to
provide a way to go from initdb-time state to
whatever-the-current-privs-are.  Dropping initdb-time privs and
reverting back to a no-privs default wouldn't be right in any case
where the initdb-time privs are different from no-privs.

As an example- if someone dropped the template1 database and then
re-created it, it won't have the same privileges as the initdb-time
template1 and instead wouldn't have any privileges- but an actual new
installation would bring back template1 with the initdb-time privileges.

Basically, the pg_dump output in that case *should* include the
equivilant of "REVOKE initdb-time privs" if we want it to be a minimal
change from what is there at initdb-time, but the only way to have that
happen is to know what the initdb-time privs were, and that's not what
will be in pg_init_privs if you drop the entries from pg_init_priv when
template1 is dropped.

I'm not sure where/why tablespaces came into this discussion at all
since we don't have any initdb-time tablespaces that can be dropped or
recreated, and the ones we do have exist with no-privs at initdb-time,
so I don't think there's any reason we'd need to have entries in
pg_init_privs for those, and it seems like they should be able to just
use the buildACLQueries magic, though it looks like you might have to
add a flag that will basically be the same as the binary_upgrade flag to
say "this object doesn't have any init privs, so we aren't joining
against pg_init_privs, and don't include that in the query".

Unfortunately, that isn't going to work for databases though.  I haven't
got any particularly great solution there.  We could possibly have a
catalog which defined the initdb-time objects using their name instead
of the initdb-time OID but that seems awful grotty..  Or we could try to
do the same thing the user did and just DROP template1 and then recreate
it, in which case it'd be created with no-privs and then do the same as
tablespaces above, if it has a too-high OID, but again, that seems
pretty ugly.

Open to other thoughts, but I really don't think we can just stick
things into pg_init_privs for global objects and then remove them if
that object is dropped, and it isn't a shared catalog either, so there's
also that...  Having a shared catalog just in case someone wants to
drop/recreate template1 strikes me as pretty massive overkill.

Thanks,

Stephen