Thread: pg_tablespace_size()
Following on from Hiroshi's earlier post: http://archives.postgresql.org/pgsql-hackers/2007-10/msg00324.php Per http://archives.postgresql.org/pgsql-hackers/2007-08/msg01018.php, superusers should be able to use pg_tablespace_size() on any tablespace, however when logged in as postgres on a beta1 server, I get: -- Executing query: select pg_tablespace_size('pg_global'); ERROR: permission denied for tablespace pg_global Can this be fixed please? Regards, Dave
On Fri, Oct 12, 2007 at 12:20:08PM +0100, Dave Page wrote: > Following on from Hiroshi's earlier post: > http://archives.postgresql.org/pgsql-hackers/2007-10/msg00324.php > > Per http://archives.postgresql.org/pgsql-hackers/2007-08/msg01018.php, > superusers should be able to use pg_tablespace_size() on any tablespace, > however when logged in as postgres on a beta1 server, I get: > > -- Executing query: > select pg_tablespace_size('pg_global'); > > ERROR: permission denied for tablespace pg_global > > Can this be fixed please? Attached is a patch I want to apply for this. Toms message at http://archives.postgresql.org/pgsql-hackers/2007-10/msg00448.php makes me bring it up here before I apply it. I think the correct behaviour is that superusers should be able to do it, and that's in the thread that Dave referred to. But since Tom said it's "by design" in the message above, I'd like confirmation that this is ok. Since I think that allowing superusers to do it is the only sensible thing, I will apply this patch unless someone objects :-) //Magnus
Attachment
Dave Page <dpage@postgresql.org> writes:
> select pg_tablespace_size('pg_global');
> ERROR: permission denied for tablespace pg_global
> Can this be fixed please?
What's to be fixed? That's exactly the result I'd expect given the
previously-agreed-to behavior for pg_tablespace_size.
regards, tom lane
Tom Lane wrote:
> Dave Page <dpage@postgresql.org> writes:
> > select pg_tablespace_size('pg_global');
> > ERROR: permission denied for tablespace pg_global
>
> > Can this be fixed please?
>
> What's to be fixed? That's exactly the result I'd expect given the
> previously-agreed-to behavior for pg_tablespace_size.
I know we said tablespace requires CREATE privs, but certainly the
super-user should be able to use the function too.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote:
> Dave Page <dpage@postgresql.org> writes:
>> select pg_tablespace_size('pg_global');
>> ERROR: permission denied for tablespace pg_global
>
>> Can this be fixed please?
>
> What's to be fixed? That's exactly the result I'd expect given the
> previously-agreed-to behavior for pg_tablespace_size.
In the message I quoted at the end of the long discussion on the topic
you assured me it would work for superusers.
Is there any reason the superuser shouldn't see the size of pg_global?
Regards, Dave.
Magnus Hagander <magnus@hagander.net> writes: > Attached is a patch I want to apply for this. Toms message at > http://archives.postgresql.org/pgsql-hackers/2007-10/msg00448.php makes me > bring it up here before I apply it. [ squint... ] There is something wrong here, because a superuser should certainly pass the aclcheck test. I don't know where the bug is but this is not the correct fix. regards, tom lane
Dave Page <dpage@postgresql.org> writes:
> In the message I quoted at the end of the long discussion on the topic
> you assured me it would work for superusers.
> Is there any reason the superuser shouldn't see the size of pg_global?
Oh, you were superuser? Then something's broken, agreed.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Attached is a patch I want to apply for this. Toms message at >> http://archives.postgresql.org/pgsql-hackers/2007-10/msg00448.php makes me >> bring it up here before I apply it. > > [ squint... ] There is something wrong here, because a superuser should > certainly pass the aclcheck test. I don't know where the bug is but > this is not the correct fix. Are you sure? pg_tablespace_aclmask() has: /* * Only shared relations can be stored in global space; don't let even * superusers override this */ if (spc_oid == GLOBALTABLESPACE_OID && !IsBootstrapProcessingMode()) return0; And this is what causes the failure. I certainly didn't want to take out that check, so short-circuiting it in the way I did seemed right.. //Magnus
I wrote:
> [ squint... ] There is something wrong here, because a superuser should
> certainly pass the aclcheck test. I don't know where the bug is but
> this is not the correct fix.
OK, after looking, the issue is this wart in pg_tablespace_aclmask():
/* * Only shared relations can be stored in global space; don't let even * superusers override this */ if
(spc_oid== GLOBALTABLESPACE_OID && !IsBootstrapProcessingMode()) return 0;
/* Otherwise, superusers bypass all permission checking. */
There are a number of ways that we could deal with this:
* Just remove the above-quoted lines. Superusers should be allowed to
shoot themselves in the foot. (I'm not actually sure that there would
be any bad consequences from putting an ordinary table into pg_global
anyway. I think I wrote the above code in fear that some parts of the
system would equate reltablespace = pg_global with relisshared, but
AFAICS that's not the case now.)
* Remove the above lines and instead put a defense into heap_create.
This might be better design anyway since a more specific error could
be reported.
* Leave aclchk.c as-is and apply Magnus' patch to allow superusers
to bypass the check in pg_tablespace_size.
* Decide that we should allow anyone to do pg_tablespace_size('pg_global')
and put in a special wart for that in dbsize.c. This wasn't part of
the original agreement but maybe there's a case to be made for it.
Thoughts?
regards, tom lane
Tom Lane wrote:
> I wrote:
>> [ squint... ] There is something wrong here, because a superuser should
>> certainly pass the aclcheck test. I don't know where the bug is but
>> this is not the correct fix.
>
> OK, after looking, the issue is this wart in pg_tablespace_aclmask():
>
> /*
> * Only shared relations can be stored in global space; don't let even
> * superusers override this
> */
> if (spc_oid == GLOBALTABLESPACE_OID && !IsBootstrapProcessingMode())
> return 0;
>
> /* Otherwise, superusers bypass all permission checking. */
Yup, that was my point.
> There are a number of ways that we could deal with this:
>
> * Just remove the above-quoted lines. Superusers should be allowed to
> shoot themselves in the foot. (I'm not actually sure that there would
> be any bad consequences from putting an ordinary table into pg_global
> anyway. I think I wrote the above code in fear that some parts of the
> system would equate reltablespace = pg_global with relisshared, but
> AFAICS that's not the case now.)
Is there ever *any* reason for doing this?
If there isn't, I don't think we should provide just that foot-gun. But
if there is any case where it makes sense to do that, then the superuser
should probably be allowed to do it.
> * Remove the above lines and instead put a defense into heap_create.
> This might be better design anyway since a more specific error could
> be reported.
>
> * Leave aclchk.c as-is and apply Magnus' patch to allow superusers
> to bypass the check in pg_tablespace_size.
See foot-gun above. If we want to keep the check, I think that my patch
is fine. If we don't, then taking out that code is better.
> * Decide that we should allow anyone to do pg_tablespace_size('pg_global')
> and put in a special wart for that in dbsize.c. This wasn't part of
> the original agreement but maybe there's a case to be made for it.
That's pretty much the same thing, right? Since the acl check will check
for pg_global, and if it's anything else, let superuser in. It's gotta
be easier to read if it's just a plain superuser check, I think.
//Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> * Just remove the above-quoted lines. Superusers should be allowed to
>> shoot themselves in the foot. (I'm not actually sure that there would
>> be any bad consequences from putting an ordinary table into pg_global
>> anyway.
> Is there ever *any* reason for doing this?
Probably not a good one, and I suspect there would be some funny
misbehaviors if you were to clone the database containing the table.
The table would be physically shared but logically not.
What I'm inclined to do about it is is adopt my suggestion #2 (move the
location of the defense), since "permission denied" for a superuser is
a pretty unhelpful error message anyway.
>> * Decide that we should allow anyone to do pg_tablespace_size('pg_global')
>> and put in a special wart for that in dbsize.c. This wasn't part of
>> the original agreement but maybe there's a case to be made for it.
> That's pretty much the same thing, right?
Well, no, I was suggesting that we might want to special-case pg_global
as a tablespace that anyone (superuser or no) could get the size of.
This is actually independent of whether we change the aclmask behavior.
regards, tom lane
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> Tom Lane wrote:
>>> * Just remove the above-quoted lines. Superusers should be allowed to
>>> shoot themselves in the foot. (I'm not actually sure that there would
>>> be any bad consequences from putting an ordinary table into pg_global
>>> anyway.
>
>> Is there ever *any* reason for doing this?
>
> Probably not a good one, and I suspect there would be some funny
> misbehaviors if you were to clone the database containing the table.
> The table would be physically shared but logically not.
yuck.
> What I'm inclined to do about it is is adopt my suggestion #2 (move the
> location of the defense), since "permission denied" for a superuser is
> a pretty unhelpful error message anyway.
Ok. Works for me.
>>> * Decide that we should allow anyone to do pg_tablespace_size('pg_global')
>>> and put in a special wart for that in dbsize.c. This wasn't part of
>>> the original agreement but maybe there's a case to be made for it.
>
>> That's pretty much the same thing, right?
>
> Well, no, I was suggesting that we might want to special-case pg_global
> as a tablespace that anyone (superuser or no) could get the size of.
> This is actually independent of whether we change the aclmask behavior.
Oh, ok, I see. Then my vote is for the other solution = not allowing
anybody to do this.
//Magnus