Re: Inconsistent syntax in GRANT

Folks,

Just got tripped up by this:

GRANT SELECT ON table1 TO someuser;
GRANT SELECT ON table1_id_seq TO someuser;
.... both work

However,
GRANT SELECT ON TABLE table1 TO someuser;
... works, while ....
GRANT SELECT ON SEQUENCE table1_id_seq TO someuser;
... raises an error.

This is inconsistent. Do people agree with me that the parser should
accept "SEQUENCE" there, since the optional object name works for all
other objects? Is there some technical reason this is difficult to do?

--
--Josh

Josh Berkus
Aglio Database Solutions
San Francisco

--- Josh Berkus <josh(at)agliodbs(dot)com> escreveu:

> However,
> GRANT SELECT ON TABLE table1 TO someuser;
> ... works, while ....
> GRANT SELECT ON SEQUENCE table1_id_seq TO someuser;
> ... raises an error.
>
> This is inconsistent. Do people agree with me that the parser
> should
> accept "SEQUENCE" there, since the optional object name works for all
>
> other objects? Is there some technical reason this is difficult to
> do?
>
It should but it's not implemented yet. There is no difficulty in doing
it. But I want to propose the following idea: if some object depends on
another object and its type is 'DEPENDENCY_INTERNAL' we could
grant/revoke privileges automagically to it. Or maybe create another
type of dependency to do so.
Comments?

Euler Taveira de Oliveira
euler[at]yahoo_com_br

_______________________________________________________
Yahoo! doce lar. Faça do Yahoo! sua homepage.
http://br.yahoo.com/homepageset.html

Euler,

> It should but it's not implemented yet. There is no difficulty in doing
> it. But I want to propose the following idea: if some object depends on
> another object and its type is 'DEPENDENCY_INTERNAL' we could
> grant/revoke privileges automagically to it. Or maybe create another
> type of dependency to do so.
> Comments?

I think this would be difficult to work out. Personally, the only
clear-cut case I can think of is SERIAL columns; other dependancies would
require a lot of conditional logic.

--
--Josh

Josh Berkus
Aglio Database Solutions
San Francisco

Josh Berkus wrote:
> Euler,
>
> > It should but it's not implemented yet. There is no difficulty in doing
> > it. But I want to propose the following idea: if some object depends on
> > another object and its type is 'DEPENDENCY_INTERNAL' we could
> > grant/revoke privileges automagically to it. Or maybe create another
> > type of dependency to do so.
> > Comments?
>
> I think this would be difficult to work out. Personally, the only
> clear-cut case I can think of is SERIAL columns; other dependancies would
> require a lot of conditional logic.

Addded to TODO:

* Allow SERIAL sequences to inherit permissions from the base table?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Josh Berkus wrote:
> Folks,
>
> Just got tripped up by this:
>
> GRANT SELECT ON table1 TO someuser;
> GRANT SELECT ON table1_id_seq TO someuser;
> .... both work
>
> However,
> GRANT SELECT ON TABLE table1 TO someuser;
> ... works, while ....
> GRANT SELECT ON SEQUENCE table1_id_seq TO someuser;
> ... raises an error.
>
> This is inconsistent. Do people agree with me that the parser should
> accept "SEQUENCE" there, since the optional object name works for all
> other objects? Is there some technical reason this is difficult to do?

The following patch allows VIEW and SEQUENCE for GRANT. I didn't add
checks for relkind, figuring it wasn't worth it, right?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> The following patch allows VIEW and SEQUENCE for GRANT. I didn't add
> checks for relkind, figuring it wasn't worth it, right?

The permissions for a sequence aren't the same as they are for a table.
We've sort of ignored the point to date, but if we're going to add
special syntax for granting on a sequence, I don't think we should
continue to ignore it.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > The following patch allows VIEW and SEQUENCE for GRANT. I didn't add
> > checks for relkind, figuring it wasn't worth it, right?
>
> The permissions for a sequence aren't the same as they are for a table.
> We've sort of ignored the point to date, but if we're going to add
> special syntax for granting on a sequence, I don't think we should
> continue to ignore it.

Uh, how are they different? You mean just UPDATE and none of the
others do anything?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce, Tom,

> > The permissions for a sequence aren't the same as they are for a
> > table. We've sort of ignored the point to date, but if we're going to
> > add special syntax for granting on a sequence, I don't think we should
> > continue to ignore it.
>
> Uh, how are they different? You mean just UPDATE and none of the
> others do anything?

Yes, it would be nice to have real permissions for sequences, specifically
USE (which allows nextval() and currval()) and UPDATE (which would allow
setval() ). However, I don't know that the added functionality would
justify breaking backwards-compatibility.

Oh, and Bruce, I can't imagine needing specific relkind so I think that
part's fine.

--
--Josh

Josh Berkus
Aglio Database Solutions
San Francisco

Bruce Momjian wrote:
> The following patch allows VIEW and SEQUENCE for GRANT. I didn't add
> checks for relkind, figuring it wasn't worth it, right?

I think checking the relkind is pretty reasonable, and should require
only a few lines of code -- why not do it?

-Neil

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> Uh, how are they different? You mean just UPDATE and none of the
>> others do anything?

> Yes, it would be nice to have real permissions for sequences, specifically
> USE (which allows nextval() and currval()) and UPDATE (which would allow
> setval() ). However, I don't know that the added functionality would
> justify breaking backwards-compatibility.

We could maintain backwards compatibility by continuing to accept the
old equivalences when you say GRANT ON TABLE. But when you say GRANT ON
SEQUENCE, I think it should use sequence-specific privilege keywords,
and not allow the privileges that don't mean anything for sequences,
like DELETE.

I'm not sure offhand what keywords we'd want to use, but now is the time
to look at it, *before* it becomes set in stone that GRANT ON SEQUENCE
is just another spelling of GRANT ON TABLE.

(The subtext of this is that I don't have a lot of use for allowing
variant syntaxes that don't actually do anything different ...)

regards, tom lane

On Thu, Jan 05, 2006 at 11:44:24 -0800,
Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Bruce, Tom,
>
> > > The permissions for a sequence aren't the same as they are for a
> > > table. We've sort of ignored the point to date, but if we're going to
> > > add special syntax for granting on a sequence, I don't think we should
> > > continue to ignore it.
> >
> > Uh, how are they different? You mean just UPDATE and none of the
> > others do anything?
>
> Yes, it would be nice to have real permissions for sequences, specifically
> USE (which allows nextval() and currval()) and UPDATE (which would allow
> setval() ). However, I don't know that the added functionality would
> justify breaking backwards-compatibility.

It might be nice to split nextval and currval access as well. nextval access
corresponds to INSERT and currval access to SELECT.

Bruno Wolff III wrote:
> On Thu, Jan 05, 2006 at 11:44:24 -0800,
> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> > Bruce, Tom,
> >
> > > > The permissions for a sequence aren't the same as they are for a
> > > > table. We've sort of ignored the point to date, but if we're going to
> > > > add special syntax for granting on a sequence, I don't think we should
> > > > continue to ignore it.
> > >
> > > Uh, how are they different? You mean just UPDATE and none of the
> > > others do anything?
> >
> > Yes, it would be nice to have real permissions for sequences, specifically
> > USE (which allows nextval() and currval()) and UPDATE (which would allow
> > setval() ). However, I don't know that the added functionality would
> > justify breaking backwards-compatibility.
>
> It might be nice to split nextval and currval access as well. nextval access
> corresponds to INSERT and currval access to SELECT.

Uh, that is already in the code. nextval()/setval() is UPDATE, and
currval() is SELECT.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> Bruno Wolff III wrote:
> > It might be nice to split nextval and currval access as well. nextval access
> > corresponds to INSERT and currval access to SELECT.
>
> Uh, that is already in the code. nextval()/setval() is UPDATE, and
> currval() is SELECT.

This seems weird. Shouldn't nextval/currval go together and setval
separately?

Considering there's no currval() without nextval(), what point
is disallowing currval() when user is able to call nextval()?

I rather want to allow nextval/currval and disable setval as it
allows regular user to DoS the database.

--
marko

[removing Tom from CC as he bounces gmail]

Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
> >> Uh, how are they different? You mean just UPDATE and none of the
> >> others do anything?
>
> > Yes, it would be nice to have real permissions for sequences, specifically
> > USE (which allows nextval() and currval()) and UPDATE (which would allow
> > setval() ). However, I don't know that the added functionality would
> > justify breaking backwards-compatibility.
>
> We could maintain backwards compatibility by continuing to accept the
> old equivalences when you say GRANT ON TABLE. But when you say GRANT ON
> SEQUENCE, I think it should use sequence-specific privilege keywords,
> and not allow the privileges that don't mean anything for sequences,
> like DELETE.

OK.

> I'm not sure offhand what keywords we'd want to use, but now is the time
> to look at it, *before* it becomes set in stone that GRANT ON SEQUENCE
> is just another spelling of GRANT ON TABLE.

Sequences do not support INSERT, UPDATE, or DELETE, but we overload
UPDATE to control nextval()/setval(), so I just allowed SELECT and
UPDATE. I am not sure it makes any sense to allow rules, references,
and triggers on sequences. However, using ALL or TABLE keywords you can
define those permissions to a sequence.

> (The subtext of this is that I don't have a lot of use for allowing
> variant syntaxes that don't actually do anything different ...)

FYI, SQL03 defines GRANT SEQUENCE.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> FYI, SQL03 defines GRANT SEQUENCE.

Oh. Well, then that gives us precedent to go by. What do they specify
as the privileges for sequences?

regards, tom lane

Marko Kreen wrote:
> On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > Bruno Wolff III wrote:
> > > It might be nice to split nextval and currval access as well. nextval access
> > > corresponds to INSERT and currval access to SELECT.
> >
> > Uh, that is already in the code. nextval()/setval() is UPDATE, and
> > currval() is SELECT.
>
> This seems weird. Shouldn't nextval/currval go together and setval
> separately?

Uh, logically, yes, but practially currval just reads/SELECTs, while
nextval modifies/UPDATEs.

> Considering there's no currval() without nextval(), what point
> is disallowing currval() when user is able to call nextval()?

Not sure. I think SET SESSION AUTHORIZATION would make it possible.

> I rather want to allow nextval/currval and disable setval as it
> allows regular user to DoS the database.

Oh, interesting. We could easily have INSERT control that if we wanted,
but I think you have to make a clear use case to override the risk of
breaking applications.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > FYI, SQL03 defines GRANT SEQUENCE.
>
> Oh. Well, then that gives us precedent to go by. What do they specify
> as the privileges for sequences?

They don't seem to specify which actions go with which objects in the
GRANT statement, nor do they specify what permissions should control the
nextval-style statements. Seems like something they should have
specified.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> Marko Kreen wrote:
> > On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > > Bruno Wolff III wrote:
> > > > It might be nice to split nextval and currval access as well. nextval access
> > > > corresponds to INSERT and currval access to SELECT.
> > >
> > > Uh, that is already in the code. nextval()/setval() is UPDATE, and
> > > currval() is SELECT.
> >
> > This seems weird. Shouldn't nextval/currval go together and setval
> > separately?
>
> Uh, logically, yes, but practially currval just reads/SELECTs, while
> nextval modifies/UPDATEs.

Yeah, thats the mechanics behind it, but the currval() only
works if the user was already able to call nextval(), so I see
no point in separating them.

In other words: there is nothing to do with only access to currval(),
and with access to nextval() but not to currval() user loses only
in convinience.

> > Considering there's no currval() without nextval(), what point
> > is disallowing currval() when user is able to call nextval()?
>
> Not sure. I think SET SESSION AUTHORIZATION would make it possible.

/me confused, looks at docs...

Huh? I really hope you are mistaken. This would mean the sequence
state for currval() is kept per-user not per-backend. This would
make impossible to make several connections as same user. Is Postgres
really that broken?

> > I rather want to allow nextval/currval and disable setval as it
> > allows regular user to DoS the database.
>
> Oh, interesting. We could easily have INSERT control that if we wanted,
> but I think you have to make a clear use case to override the risk of
> breaking applications.

I'd turn it around: is there any use-case for setval() for regular user?
IMHO it's a admin-level operation, dangerous, and not needed for regular
work.

--
marko

Marko Kreen <markokr(at)gmail(dot)com> writes:
> On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
>> Uh, logically, yes, but practially currval just reads/SELECTs, while
>> nextval modifies/UPDATEs.

> Yeah, thats the mechanics behind it, but the currval() only
> works if the user was already able to call nextval(), so I see
> no point in separating them.

You are completely wrong on this, because not all the code in a session
necessarily executes at the same privilege level. For instance, the
nextval() might be executed inside a SECURITY DEFINER function. It
might be reasonable to give code outside that function the right to see
what had been assigned (by executing currval()) without also saying that
it could do further nextvals().

I do agree that it would be a good idea to support a privilege
distinction between nextval() and setval().

>> Oh, interesting. We could easily have INSERT control that if we wanted,
>> but I think you have to make a clear use case to override the risk of
>> breaking applications.

There is no backwards-compatibility risk, because we'd still have the
old GRANT ON TABLE syntax grant both underlying rights. You'd have to
use the new syntax to get to a state where you had nextval but not
setval privilege or vice versa.

regards, tom lane

On Fri, Jan 06, 2006 at 19:11:27 +0200,
Marko Kreen <markokr(at)gmail(dot)com> wrote:
> On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
>
> Considering there's no currval() without nextval(), what point
> is disallowing currval() when user is able to call nextval()?
>
> I rather want to allow nextval/currval and disable setval as it
> allows regular user to DoS the database.

What I was thinking with this, is that you might allow someone the ability
to insert records into a table which would make use of nextval, but not
allow them to run nextval directly. But after inserting a record allow them
to use currval to see what value was assigned.
People could still mess with things by doing INSERTs and aborting the
transaction, so this may not be the best example for why you would want this.

Bruno Wolff III wrote:
> On Fri, Jan 06, 2006 at 19:11:27 +0200,
> Marko Kreen <markokr(at)gmail(dot)com> wrote:
> > On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> >
> > Considering there's no currval() without nextval(), what point
> > is disallowing currval() when user is able to call nextval()?
> >
> > I rather want to allow nextval/currval and disable setval as it
> > allows regular user to DoS the database.
>
> What I was thinking with this, is that you might allow someone the ability
> to insert records into a table which would make use of nextval, but not
> allow them to run nextval directly. But after inserting a record allow them
> to use currval to see what value was assigned.
> People could still mess with things by doing INSERTs and aborting the
> transaction, so this may not be the best example for why you would want this.

That seems too confusing to support based on usefulness of the new
capability.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On 1/6/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Kreen <markokr(at)gmail(dot)com> writes:
> > On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> >> Uh, logically, yes, but practially currval just reads/SELECTs, while
> >> nextval modifies/UPDATEs.
>
> > Yeah, thats the mechanics behind it, but the currval() only
> > works if the user was already able to call nextval(), so I see
> > no point in separating them.
>
> You are completely wrong on this, because not all the code in a session
> necessarily executes at the same privilege level. For instance, the
> nextval() might be executed inside a SECURITY DEFINER function. It
> might be reasonable to give code outside that function the right to see
> what had been assigned (by executing currval()) without also saying that
> it could do further nextvals().

Ah, I did not think of this. Indeed, it's useful to keep them separate.
I just wanted to point out that I see much more use to keep setval()
separate from nextval/currval. (that is - always)

> I do agree that it would be a good idea to support a privilege
> distinction between nextval() and setval().

I tried to imagine a usage scenario for setval() but only
single-user bulk data load comes in mind. Is there any
actual scenario where it could be useful in multi-user
setting?

> >> Oh, interesting. We could easily have INSERT control that if we wanted,
> >> but I think you have to make a clear use case to override the risk of
> >> breaking applications.
>
> There is no backwards-compatibility risk, because we'd still have the
> old GRANT ON TABLE syntax grant both underlying rights. You'd have to
> use the new syntax to get to a state where you had nextval but not
> setval privilege or vice versa.

Good idea.

--
marko

On 1/6/06, Bruno Wolff III <bruno(at)wolff(dot)to> wrote:
> On Fri, Jan 06, 2006 at 19:11:27 +0200,
> Marko Kreen <markokr(at)gmail(dot)com> wrote:
> > On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> >
> > Considering there's no currval() without nextval(), what point
> > is disallowing currval() when user is able to call nextval()?
> >
> > I rather want to allow nextval/currval and disable setval as it
> > allows regular user to DoS the database.
>
> What I was thinking with this, is that you might allow someone the ability
> to insert records into a table which would make use of nextval, but not
> allow them to run nextval directly. But after inserting a record allow them
> to use currval to see what value was assigned.
> People could still mess with things by doing INSERTs and aborting the
> transaction, so this may not be the best example for why you would want this.

This is similar to Tom's scenario. I'm not against keeping them separate.

But my question is rather - is there any scenario where setval() should
go with nextval()?

It seems that their pairing is an accident and should be fixed.

--
marko

Marko Kreen <markokr(at)gmail(dot)com> writes:
> But my question is rather - is there any scenario where setval() should
> go with nextval()?

> It seems that their pairing is an accident and should be fixed.

I think the original argument for the current design was that with
enough nextval's you can duplicate the effect of a setval. This is only
strictly true if the sequence is CYCLE mode, and even then it'd take a
whole lot of patience to wrap an int8 sequence around ... but the
distinction between them is not so large as you make it out to be.

In any case I think we are wasting our time discussing it, and instead
should be looking through the SQL2003 spec to see what it requires.
Bruce couldn't find anything in it about this but I can't believe the
info isn't there somewhere.

regards, tom lane

Tom Lane wrote:
> Marko Kreen <markokr(at)gmail(dot)com> writes:
> > But my question is rather - is there any scenario where setval() should
> > go with nextval()?
>
> > It seems that their pairing is an accident and should be fixed.
>
> I think the original argument for the current design was that with
> enough nextval's you can duplicate the effect of a setval. This is only
> strictly true if the sequence is CYCLE mode, and even then it'd take a
> whole lot of patience to wrap an int8 sequence around ... but the
> distinction between them is not so large as you make it out to be.
>
> In any case I think we are wasting our time discussing it, and instead
> should be looking through the SQL2003 spec to see what it requires.
> Bruce couldn't find anything in it about this but I can't believe the
> info isn't there somewhere.

What I did was to read through the GRANT and SEQUENCE sections, then I
dumped it to text and did a grep for 'grant' or perm* appearing on the
same line as sequence, and came up with nothing.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:
> > I'm not sure offhand what keywords we'd want to use, but now is the time
> > to look at it, *before* it becomes set in stone that GRANT ON SEQUENCE
> > is just another spelling of GRANT ON TABLE.
>
> Sequences do not support INSERT, UPDATE, or DELETE, but we overload
> UPDATE to control nextval()/setval(), so I just allowed SELECT and
> UPDATE. I am not sure it makes any sense to allow rules, references,
> and triggers on sequences. However, using ALL or TABLE keywords you can
> define those permissions to a sequence.

Here is an updated patch. The standard doesn't have GRANT VIEW so I
didn't implement that.

One tricky issue I realized is that we should dump out GRANT SEQUENCE,
if possible. I have added code to check in pg_dump and use GRANT
SEQUENCE if only SELECT, UPDATE, or ALL are used.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On 1/6/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Kreen <markokr(at)gmail(dot)com> writes:
> > But my question is rather - is there any scenario where setval() should
> > go with nextval()?
>
> > It seems that their pairing is an accident and should be fixed.
>
> I think the original argument for the current design was that with
> enough nextval's you can duplicate the effect of a setval. This is only
> strictly true if the sequence is CYCLE mode, and even then it'd take a
> whole lot of patience to wrap an int8 sequence around ... but the
> distinction between them is not so large as you make it out to be.

With bigserial this is more like CPU DoS, while other users can work
normally.

> In any case I think we are wasting our time discussing it, and instead
> should be looking through the SQL2003 spec to see what it requires.
> Bruce couldn't find anything in it about this but I can't believe the
> info isn't there somewhere.

Google tells that Oracle has ALTER and SELECT; DB2 has ALTER and USAGE.

I found SQL2003 pdf's too ... from my reading it has only USAGE.

5WD-02-Foundation-2003-09.pdf:
page 724 -> General Rules -> #2
page 740 -> Syntax rules -> #3

Everything combined:
SELECT: currval
UPDATE: nextval
USAGE: currval, nextval
ALTER: setval

Confusing?

--
marko

Marko Kreen wrote:
> > In any case I think we are wasting our time discussing it, and instead
> > should be looking through the SQL2003 spec to see what it requires.
> > Bruce couldn't find anything in it about this but I can't believe the
> > info isn't there somewhere.
>
> Google tells that Oracle has ALTER and SELECT; DB2 has ALTER and USAGE.
>
> I found SQL2003 pdf's too ... from my reading it has only USAGE.
>
> 5WD-02-Foundation-2003-09.pdf:
> page 724 -> General Rules -> #2
> page 740 -> Syntax rules -> #3

I admit I am terrible at understanding the standard, but I can't find
anything relevant on the page numbers you mentioned. Are those the
document pages or the page numbers displayed by the PDF viewer? What is
the section heading? I am using the same filename you have.

> Everything combined:
> SELECT: currval
> UPDATE: nextval
> USAGE: currval, nextval
> ALTER: setval
>
> Confusing?

I see USAGE in the standard, but not ALTER. We don't support USAGE so I
am guessing our SELECT/UPDATE behavior is OK. Does this mean we should
only allow owners to do setval(), rather than binding it to INSERT?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> Marko Kreen wrote:
> > I found SQL2003 pdf's too ... from my reading it has only USAGE.
> >
> > 5WD-02-Foundation-2003-09.pdf:
> > page 724 -> General Rules -> #2
> > page 740 -> Syntax rules -> #3
>
> I admit I am terrible at understanding the standard, but I can't find
> anything relevant on the page numbers you mentioned. Are those the
> document pages or the page numbers displayed by the PDF viewer? What is
> the section heading? I am using the same filename you have.

Those are print page numbers. (In case you have dead-tree variant :)
And I got them here: http://www.wiscorp.com/SQLStandards.html

Uh, and they are bit wrong. Ok here are they fully:

11.62 <sequence generator definition>
General rules (page 727 printed/751 real) point #2

12.3 <privileges>
Syntax rules (page 740 printed/764 real) point #3

> > Everything combined:
> > SELECT: currval
> > UPDATE: nextval
> > USAGE: currval, nextval
> > ALTER: setval
> >
> > Confusing?
>
> I see USAGE in the standard, but not ALTER. We don't support USAGE so I
> am guessing our SELECT/UPDATE behavior is OK.

No, we still want to separate setval from nextval.

> Does this mean we should
> only allow owners to do setval(), rather than binding it to INSERT?

My first reaction is that it should be grantable, although
I can't find any reasons for it, except backwards compatibility.

How about this:

SELECT: currval
INSERT: nextval
USAGE: currval, nextval
UPDATE: setval

--
marko

Marko Kreen wrote:
> On 1/6/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> > Marko Kreen wrote:
> > > I found SQL2003 pdf's too ... from my reading it has only USAGE.
> > >
> > > 5WD-02-Foundation-2003-09.pdf:
> > > page 724 -> General Rules -> #2
> > > page 740 -> Syntax rules -> #3
> >
> > I admit I am terrible at understanding the standard, but I can't find
> > anything relevant on the page numbers you mentioned. Are those the
> > document pages or the page numbers displayed by the PDF viewer? What is
> > the section heading? I am using the same filename you have.
>
> Those are print page numbers. (In case you have dead-tree variant :)
> And I got them here: http://www.wiscorp.com/SQLStandards.html
>
> Uh, and they are bit wrong. Ok here are they fully:
>
> 11.62 <sequence generator definition>
> General rules (page 727 printed/751 real) point #2
>
> 12.3 <privileges>
> Syntax rules (page 740 printed/764 real) point #3

OK, I see it now, and in an earlier email I quoted the part where I
think USAGE links in to nextval(). I was looking for something obvious. :-)

> > > Everything combined:
> > > SELECT: currval
> > > UPDATE: nextval
> > > USAGE: currval, nextval
> > > ALTER: setval
> > >
> > > Confusing?
> >
> > I see USAGE in the standard, but not ALTER. We don't support USAGE so I
> > am guessing our SELECT/UPDATE behavior is OK.
>
> No, we still want to separate setval from nextval.

My point was that currval -> SELECT and nextval -> UPDATE was correct.
I see now that I am wrong and that the standard wants USAGE.

That combined was every db's behavior combined, right? I got confused.

> > Does this mean we should
> > only allow owners to do setval(), rather than binding it to INSERT?
>
> My first reaction is that it should be grantable, although
> I can't find any reasons for it, except backwards compatibility.
>
> How about this:
>
> SELECT: currval
> INSERT: nextval
> USAGE: currval, nextval
> UPDATE: setval

I think nextval() is naturally UPDATE. I am thinking setval would be
INSERT, and with setval() being used less, it would perhaps be a better
choice for a change anyway.

However, in doing the pg_dump part of the patch, I perhaps see a
problem. If someone does:

GRANT UPDATE ON seq1 TO PUBLIC;

do we give them nextval() and setval() permissions? If they do:

GRANT UPDATE ON SEQUENCE seq1 TO PUBLIC;
--------

they only set nextval()? That seems quite confusing. Can we change
UPDATE for both GRANT syntaxes, and somehow have people fix them up
after they load in 8.2? How many non-owners do setval()?

FYI, we could support USAGE just on sequences, and have it map to
UPDATE, but pg_dump it out as USAGE.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> FYI, we could support USAGE just on sequences, and have it map to
> UPDATE, but pg_dump it out as USAGE.

It seems the spec doesn't cover setval() and currval(), which is not
too surprising given those aren't standard.

Here is a proposal:

SELECT priv -> allows currval() and SELECT * FROM seq

USAGE priv -> allows nextval() (required by SQL2003)

UPDATE priv -> allows setval() and nextval()

I was originally thinking of a separate privilege bit for setval(), but
that's sort of silly, as you can get (approximately) the effect of
nextval() via setval(). Not much point in prohibiting nextval() to
someone who can do setval().

This is 100% upward compatible with our current definition, and it meets
both the SQL spec and Marko's desire to have a way of granting only
nextval() privilege.

BTW, what about lastval()? I'm not sure we can usefully associate any
privilege check with that, since it's not clear which sequence it
applies to. Does it make sense to remember what sequence the value came
from and privilege-check against that, or is that just too weird?

regards, tom lane

Tom,

> BTW, what about lastval()?  I'm not sure we can usefully associate any
> privilege check with that, since it's not clear which sequence it
> applies to.  Does it make sense to remember what sequence the value came
> from and privilege-check against that, or is that just too weird?

Hmmm. Yet another problem with lastval(). Darn those MySQL migrators!

I'd say that lastval() needs to be defined as the superuser with "security
definer". Hmmm, although does that carry over to sequences the superuser
doesn't own? How are we handling it now?

Overal, it's hard to get too concerned about this, since a user can't
really get anything out of lastval() if he doesn't have permissions on the
sequence he's trying to query, in order to run currval.

--
--Josh

Josh Berkus
Aglio Database Solutions
San Francisco

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> BTW, what about lastval()?

> Overal, it's hard to get too concerned about this, since a user can't
> really get anything out of lastval() if he doesn't have permissions on the
> sequence he's trying to query, in order to run currval.

Well, no, consider my example to Marko: there could be a SECURITY
DEFINER function that has the privilege to run nextval(). After
that, if lastval() isn't privilege-checked then code that doesn't
have any privilege at all on the sequence could get at the value.

However, looking at the source code I see that lastval() does in fact
insist on SELECT rights on the sequence the value is coming from.
So I guess we can just leave that as-is.

regards, tom lane

On 1/7/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > FYI, we could support USAGE just on sequences, and have it map to
> > UPDATE, but pg_dump it out as USAGE.
>
> It seems the spec doesn't cover setval() and currval(), which is not
> too surprising given those aren't standard.
>
> Here is a proposal:
>
> SELECT priv -> allows currval() and SELECT * FROM seq
>
> USAGE priv -> allows nextval() (required by SQL2003)
>
> UPDATE priv -> allows setval() and nextval()
>
> I was originally thinking of a separate privilege bit for setval(), but
> that's sort of silly, as you can get (approximately) the effect of
> nextval() via setval(). Not much point in prohibiting nextval() to
> someone who can do setval().
>
> This is 100% upward compatible with our current definition, and it meets
> both the SQL spec and Marko's desire to have a way of granting only
> nextval() privilege.

Good point about compatibility. But makes the common case ugly.
"For regular usage you need to grant SELECT, USAGE ..." Huh? :)

How about this:

SELECT: currval
INSERT: nextval
UPDATE: nextval, setval
USAGE: nextval, currval

With this the user needs only to remember SQL2003 syntax
to cover 99.9% use cases. And when he wants to play more
finegrained then he can combine with the SELECT, INSERT, UPDATE.

The above table seem bit messy, but I see it as much easier to explain
to somebody.

> BTW, what about lastval()? I'm not sure we can usefully associate any
> privilege check with that, since it's not clear which sequence it
> applies to. Does it make sense to remember what sequence the value came
> from and privilege-check against that, or is that just too weird?

Hmm. So it means with lastval() user can see the state of sequences
he has no access to? Seems like the privilege check would be good
idea.

--
marko

Marko Kreen wrote:
> On 1/7/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > FYI, we could support USAGE just on sequences, and have it map to
> > > UPDATE, but pg_dump it out as USAGE.
> >
> > It seems the spec doesn't cover setval() and currval(), which is not
> > too surprising given those aren't standard.
> >
> > Here is a proposal:
> >
> > SELECT priv -> allows currval() and SELECT * FROM seq
> >
> > USAGE priv -> allows nextval() (required by SQL2003)
> >
> > UPDATE priv -> allows setval() and nextval()
> >
> > I was originally thinking of a separate privilege bit for setval(), but
> > that's sort of silly, as you can get (approximately) the effect of
> > nextval() via setval(). Not much point in prohibiting nextval() to
> > someone who can do setval().
> >
> > This is 100% upward compatible with our current definition, and it meets
> > both the SQL spec and Marko's desire to have a way of granting only
> > nextval() privilege.
>
> Good point about compatibility. But makes the common case ugly.
> "For regular usage you need to grant SELECT, USAGE ..." Huh? :)
>
> How about this:
>
> SELECT: currval
> INSERT: nextval
> UPDATE: nextval, setval
> USAGE: nextval, currval
>
> With this the user needs only to remember SQL2003 syntax
> to cover 99.9% use cases. And when he wants to play more
> finegrained then he can combine with the SELECT, INSERT, UPDATE.

I think we should use Tom's suggestion, for two reasons. First, the
common case currently needs both SELECT and UPDATE, and I have heard no
one complain about it. Second, I think USAGE is better assocated with
nextval() and UPDATE with both nextval() and setval().

> The above table seem bit messy, but I see it as much easier to explain
> to somebody.

I am confused about your list above, so I can't see how that would be
easy to explain.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Marko Kreen <markokr(at)gmail(dot)com> writes:
> Good point about compatibility. But makes the common case ugly.
> "For regular usage you need to grant SELECT, USAGE ..." Huh? :)

> How about this:

> SELECT: currval
> INSERT: nextval
> UPDATE: nextval, setval
> USAGE: nextval, currval

Seems a little weird. Hmm ... what is the use-case for allowing someone
to do nextval but not currval? I can't see one. How about we simplify
this to

SELECT: currval
UPDATE: nextval, setval
USAGE: nextval, currval

This is still upward compatible with our old behavior, which is

SELECT: currval
UPDATE: nextval, setval

and it still meets the SQL spec's requirement that USAGE allow nextval,
and USAGE is the only one you need for "normal" usage.

regards, tom lane

Tom Lane wrote:
> Marko Kreen <markokr(at)gmail(dot)com> writes:
> > Good point about compatibility. But makes the common case ugly.
> > "For regular usage you need to grant SELECT, USAGE ..." Huh? :)
>
> > How about this:
>
> > SELECT: currval
> > INSERT: nextval
> > UPDATE: nextval, setval
> > USAGE: nextval, currval
>
> Seems a little weird. Hmm ... what is the use-case for allowing someone
> to do nextval but not currval? I can't see one. How about we simplify
> this to
>
> SELECT: currval
> UPDATE: nextval, setval
> USAGE: nextval, currval
>
> This is still upward compatible with our old behavior, which is
>
> SELECT: currval
> UPDATE: nextval, setval
>
> and it still meets the SQL spec's requirement that USAGE allow nextval,
> and USAGE is the only one you need for "normal" usage.

I think your original proposal was better. Why is it important that we
have a single-keyword usage for the common case? No one has complained
about what we have now and that requires two keywords just like your
proposal. We don't have a shorthand for GRANT INSERT/UPDATE/DELETE. In
fact, if it was backward-compatible I would suggest we make UPDATE just
setval. Does the standard require USAGE to support currval?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Does the standard require USAGE to support currval?

currval isn't in the standard (unless I missed something), so it has
nothing to say one way or the other on the point.

Basically what we seem to be homing in on is to keep SELECT and UPDATE
privileges doing what they do now and then add a USAGE privilege.
I think I agree with Marko that USAGE should mean nextval + currval;
it already must overlap UPDATE and so there's no very good reason why it
shouldn't overlap SELECT too. Furthermore there's no plausible use-case
where you'd want to grant nextval but not currval, so why not keep the
notation simple?

regards, tom lane

I wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>> Does the standard require USAGE to support currval?

> currval isn't in the standard (unless I missed something), so it has
> nothing to say one way or the other on the point.

Wait, I take that back. Remember our previous discussions about this
point: the spec's NEXT VALUE FOR construct is *not* equivalent to
nextval, because they specify that the sequence advances just once per
command even if the command says NEXT VALUE FOR in multiple places.
This means that NEXT VALUE FOR is effectively both nextval and currval;
the first one in a command does nextval and the rest do currval.

Accordingly, I think it's reasonable to read the spec as saying that
USAGE privilege encompasses both nextval and currval.

regards, tom lane

Should UPDATE also allow currval()? Your logic below seems to suggest
that.

---------------------------------------------------------------------------

Tom Lane wrote:
> I wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> >> Does the standard require USAGE to support currval?
>
> > currval isn't in the standard (unless I missed something), so it has
> > nothing to say one way or the other on the point.
>
> Wait, I take that back. Remember our previous discussions about this
> point: the spec's NEXT VALUE FOR construct is *not* equivalent to
> nextval, because they specify that the sequence advances just once per
> command even if the command says NEXT VALUE FOR in multiple places.
> This means that NEXT VALUE FOR is effectively both nextval and currval;
> the first one in a command does nextval and the rest do currval.
>
> Accordingly, I think it's reasonable to read the spec as saying that
> USAGE privilege encompasses both nextval and currval.
>
> regards, tom lane
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Tom Lane wrote:
> I wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> >> Does the standard require USAGE to support currval?
>
> > currval isn't in the standard (unless I missed something), so it has
> > nothing to say one way or the other on the point.
>
> Wait, I take that back. Remember our previous discussions about this
> point: the spec's NEXT VALUE FOR construct is *not* equivalent to
> nextval, because they specify that the sequence advances just once per
> command even if the command says NEXT VALUE FOR in multiple places.
> This means that NEXT VALUE FOR is effectively both nextval and currval;
> the first one in a command does nextval and the rest do currval.
>
> Accordingly, I think it's reasonable to read the spec as saying that
> USAGE privilege encompasses both nextval and currval.

Here's a patch that more closely matches the ideas proposed.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Should UPDATE also allow currval()? Your logic below seems to suggest
> that.

I thought about that, but there are a couple of reasons not to:

1. It'd be a change from the current behavior of UPDATE privilege.
2. If there's someone out there who really does want write-only
privileges for sequences, they'd be out in the cold.

I don't find either of these very compelling, but the case for changing
the behavior of UPDATE isn't strong either. I think backwards
compatibility should carry the day if there's not a strong argument
in favor of change.

regards, tom lane

On 1/7/06, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> wrote:
> Marko Kreen wrote:
> > The above table seem bit messy, but I see it as much easier to explain
> > to somebody.
>
> I am confused about your list above, so I can't see how that would be
> easy to explain.

Easy as in "use GRANT USAGE, forget about rest". You are confused
because you know the old way and look them together.

I would have liked to say "the rest are for fine-grained access control",
but with Tom's final proposal, the explanation would continue "SELECT,
UPDATE are for backwards compatibility".

Attached is a patch that fixes tablename->seqname and puts USAGE
as first in list to show it's the preferred way. I think it should
be mentioned somewhere explicitly, but I cant find proper place for
it. In the Compatibility section for GRANT?

--
marko

Tom, all,

> SELECT: currval
> UPDATE: nextval, setval
> USAGE: nextval, currval

+1.

--Josh

Bruce Momjian wrote:
> Tom Lane wrote:
> > I wrote:
> > > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > >> Does the standard require USAGE to support currval?
> >
> > > currval isn't in the standard (unless I missed something), so it has
> > > nothing to say one way or the other on the point.
> >
> > Wait, I take that back. Remember our previous discussions about this
> > point: the spec's NEXT VALUE FOR construct is *not* equivalent to
> > nextval, because they specify that the sequence advances just once per
> > command even if the command says NEXT VALUE FOR in multiple places.
> > This means that NEXT VALUE FOR is effectively both nextval and currval;
> > the first one in a command does nextval and the rest do currval.
> >
> > Accordingly, I think it's reasonable to read the spec as saying that
> > USAGE privilege encompasses both nextval and currval.
>
> Here's a patch that more closely matches the ideas proposed.

Here is an updated patch. I hit a few issues.

At first I was just going to continue allowing table-like permissions
for sequences if a GRANT [TABLE] was used, and add the new
USAGE/SELECT/UPDATE capability only for GRANT SEQUENCE. The problem was
that you could create a non-dumpable permission setup if you added
DELETE permission to a sequence using GRANT TABLE, and USAGE permission
using GRANT SEQUENCE. That couldn't be dumped with TABLE or with
SEQUENCE, and I didn't want to do a double-dump of GRANT to fit that,
nor did I want to throw an warning during the dump run.

What I did was to throw a warning if an invalid permission is specified
for a sequence in GRANT TABLE. By doing this, un-dumpable permission
combinations will not be loaded into an 8.2 database. (GRANT ALL ON
TABLE sets the sequence-only permissions.)

test=> GRANT DELETE ON seq TO PUBLIC;
WARNING: invalid privilege type DELETE for sequence
WARNING: no privileges were granted
GRANT

test=> GRANT DELETE,SELECT ON seq TO PUBLIC;
WARNING: invalid privilege type DELETE for sequence
GRANT

This seemed the safest backward-compatible setup. It will have to be
mentioned in the release notes so users know they might get warnings
from loading sequences into 8.2.

You might think that it is unlikely for a DELETE permission to be
assigned to a sequences, but a simple GRANT ALL and REVOKE INSERT in 8.1
will cause:

test=> CREATE TABLE tab(x INTEGER);
CREATE TABLE
test=> GRANT ALL ON tab TO PUBLIC;
GRANT
test=> REVOKE INSERT ON tab FROM PUBLIC;
REVOKE

yields in pg_dump output:

GRANT SELECT,RULE,UPDATE,DELETE,REFERENCES,TRIGGER ON TABLE tab
TO PUBLIC;

This test was done on a table, but in 8.1 the same would appear for a
sequence with these warnings on load into 8.2:

WARNING: invalid privilege type RULE for sequence
WARNING: invalid privilege type DELETE for sequence
WARNING: invalid privilege type REFERENCES for sequence
WARNING: invalid privilege type TRIGGER for sequence
GRANT

Another tricky case was this:

test=> GRANT DELETE ON tab, seq TO PUBLIC;

GRANT allows multiple objects to be listed, as illustrated above. The
current code checks for valid permissions in one place because it
assumes all listed objects are of the same type and accept the same
permissions. Because GRANT TABLE must allow only valid permissions for
sequences (to avoid un-dumpable output) I had to throw an error if a
sequence is mixed with a non-sequence, and the permission did not apply
to both sequences and non-sequences, rather than throw a warning like I
usually do for invalid sequence permissions:

test=> REVOKE DELETE ON seq, tab FROM PUBLIC;
WARNING: invalid privilege type DELETE for sequence
ERROR: DELETE privilege invalid for command mixing sequences and non-sequences

test=> REVOKE SELECT ON tab, seq FROM PUBLIC;
REVOKE

Because allowing sequences to use GRANT TABLE is only for backward
compatibility, I think this is fine. If not, we would have to split
apart the permission checking for tables from the existing routine, and
lose modularity in the code.

This patch also contains Marko's documentation adjustments.

Would someone look at the change in src/backend/catalog/pg_shdepend.c
for shared dependencies? We don't have any system catalog sequences let
alone any shared catalog sequences, so I assume we are OK with assuming
it is a relation. I added a comment just in case.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> At first I was just going to continue allowing table-like permissions
> for sequences if a GRANT [TABLE] was used, and add the new
> USAGE/SELECT/UPDATE capability only for GRANT SEQUENCE. The problem was
> that you could create a non-dumpable permission setup if you added
> DELETE permission to a sequence using GRANT TABLE, and USAGE permission
> using GRANT SEQUENCE. That couldn't be dumped with TABLE or with
> SEQUENCE, and I didn't want to do a double-dump of GRANT to fit that,
> nor did I want to throw an warning during the dump run.

Just ignore the inapplicable permissions during pg_dump. I think you're
making this harder than it needs to be...

> test=> REVOKE DELETE ON seq, tab FROM PUBLIC;
> WARNING: invalid privilege type DELETE for sequence
> ERROR: DELETE privilege invalid for command mixing sequences and non-sequences

This is just plain silly. If you're going to go to that length, why not
rearrange the code to avoid the problem instead?

> Would someone look at the change in src/backend/catalog/pg_shdepend.c
> for shared dependencies? We don't have any system catalog sequences let
> alone any shared catalog sequences, so I assume we are OK with assuming
> it is a relation.

We might have such in the future though.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > At first I was just going to continue allowing table-like permissions
> > for sequences if a GRANT [TABLE] was used, and add the new
> > USAGE/SELECT/UPDATE capability only for GRANT SEQUENCE. The problem was
> > that you could create a non-dumpable permission setup if you added
> > DELETE permission to a sequence using GRANT TABLE, and USAGE permission
> > using GRANT SEQUENCE. That couldn't be dumped with TABLE or with
> > SEQUENCE, and I didn't want to do a double-dump of GRANT to fit that,
> > nor did I want to throw an warning during the dump run.
>
> Just ignore the inapplicable permissions during pg_dump. I think you're
> making this harder than it needs to be...

I don't think we should allow GRANT DELETE ON seq in 8.2 for invalid
permission. If we are going to say what permissions are supported by
sequences, we should enforce that rather than silently accept it, and
once we decide that, we need infrastructure to treat sequences and
non-sequences differently.

The dump illustration is just another example of why we have to tighten
this. I would be OK to allow it and preserve it, but not to allow it
and ignore it in a dump, basically throwing it away from dump to reload.
The fact we can't preserve it drove me in this direction.

> > test=> REVOKE DELETE ON seq, tab FROM PUBLIC;
> > WARNING: invalid privilege type DELETE for sequence
> > ERROR: DELETE privilege invalid for command mixing sequences and non-sequences
>
> This is just plain silly. If you're going to go to that length, why not
> rearrange the code to avoid the problem instead?

Ignoring your insult, the code is structured this way:

check all permission bits
call object-type-specific routine
loop over each object and set permission bits

so, to fix this, I would need to move the permission bit checks into
object-type-specific routines so that I could check the permission bits
for each object, rather than once in a single place. You could still do
that single permission check in each object-type-specific routine and
just do the loop for the GRANT TABLE case. Does that seem worth it?
Another solution would be to save the permission bits in a List one per
object rather than as a single value for all objects, but that mucks up
the API for all objects just to catch the sequence case of GRANT TABLE.

> > Would someone look at the change in src/backend/catalog/pg_shdepend.c
> > for shared dependencies? We don't have any system catalog sequences let
> > alone any shared catalog sequences, so I assume we are OK with assuming
> > it is a relation.
>
> We might have such in the future though.

The problem is that the case statement triggers off of
RelationRelationId:

switch (sdepForm->classid)
{
case RelationRelationId:
/* could be a sequence? */
istmt.objtype = ACL_OBJECT_RELATION;

The problem is that pg_class holds both sequences and non-sequences, so
I am suspecting the fix will require another field in the pg_shdepend
table, which seems wasteful considering we have no shared catalog
sequences.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> Just ignore the inapplicable permissions during pg_dump. I think you're
>> making this harder than it needs to be...

> I don't think we should allow GRANT DELETE ON seq in 8.2 for invalid
> permission.

That's fine, but pg_dump has to continue to work against old servers,
so it's going to have to be coded to ignore inapplicable permissions
anyway. Contorting the server-side code to avoid that is pointless.

> Ignoring your insult, the code is structured this way:

> check all permission bits
> call object-type-specific routine
> loop over each object and set permission bits

> so, to fix this, I would need to move the permission bit checks into
> object-type-specific routines so that I could check the permission bits
> for each object, rather than once in a single place.

You'd have to allow the union of relation and sequence rights during the
conversion to bitmask form in ExecuteGrantStmt, and then check more
closely inside the per-object loop in ExecGrant_Relation, but that
doesn't seem like a showstopper to me. It certainly seems more pleasant
than exposing bizarre restrictions to users because we're sharing code
between the cases.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> Just ignore the inapplicable permissions during pg_dump. I think you're
> >> making this harder than it needs to be...
>
> > check all permission bits
> > call object-type-specific routine
> > loop over each object and set permission bits
>
> > so, to fix this, I would need to move the permission bit checks into
> > object-type-specific routines so that I could check the permission bits
> > for each object, rather than once in a single place.
>
> You'd have to allow the union of relation and sequence rights during the
> conversion to bitmask form in ExecuteGrantStmt, and then check more
> closely inside the per-object loop in ExecGrant_Relation, but that
> doesn't seem like a showstopper to me. It certainly seems more pleasant
> than exposing bizarre restrictions to users because we're sharing code
> between the cases.

Your idea of using a union of permission bits was very helpful. I was
afraid I was going to have to loop over every permission bit again in
the table/sequence grant permission code, but the union allowed for a
very simple check in that code.

It allows for better code checks and I think it behaves as expected:

test=> CREATE TABLE tab(x INTEGER);
CREATE TABLE
test=> CREATE SEQUENCE seq;
CREATE SEQUENCE
test=> GRANT ALL ON seq, tab TO PUBLIC;
GRANT
test=> REVOKE USAGE ON seq, tab FROM PUBLIC;
ERROR: invalid privilege type USAGE for table
test=> REVOKE SELECT ON seq, tab FROM PUBLIC;
REVOKE
test=> REVOKE DELETE ON seq, tab FROM PUBLIC;
WARNING: sequence "seq" only supports USAGE, SELECT, AND UPDATE
WARNING: no privileges could be revoked for "seq"
REVOKE

and pg_dump has:

GRANT USAGE,UPDATE ON SEQUENCE x TO PUBLIC;

GRANT INSERT,RULE,UPDATE,REFERENCES,TRIGGER ON TABLE xx TO PUBLIC;

Note I had to add the object name to the warning message so it is clear
which object permission changes did succeed. I have updated the patch.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Updated patch applied. I decided Tom was right to just ignore invalid
sequence permission from pre-8.2 databases, rather than try to use GRANT
TABLE; there was no reason to do it and avoiding it made the code
cleaner and more robust.

The changes were:

Add GRANT ON SEQUENCE syntax to support sequence-only permissions.
Continue to support GRANT ON [TABLE] for sequences for backward
compatibility; issue warning for invalid sequence permissions.

[ Backward compatibility warning message.]

Add USAGE permission for sequences that allows only currval() and
nextval(), not setval().

Mention object name in grant/revoke warnings because of possible
multi-object operations.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > Tom Lane wrote:
> > >> Just ignore the inapplicable permissions during pg_dump. I think you're
> > >> making this harder than it needs to be...
> >
> > > check all permission bits
> > > call object-type-specific routine
> > > loop over each object and set permission bits
> >
> > > so, to fix this, I would need to move the permission bit checks into
> > > object-type-specific routines so that I could check the permission bits
> > > for each object, rather than once in a single place.
> >
> > You'd have to allow the union of relation and sequence rights during the
> > conversion to bitmask form in ExecuteGrantStmt, and then check more
> > closely inside the per-object loop in ExecGrant_Relation, but that
> > doesn't seem like a showstopper to me. It certainly seems more pleasant
> > than exposing bizarre restrictions to users because we're sharing code
> > between the cases.
>
> Your idea of using a union of permission bits was very helpful. I was
> afraid I was going to have to loop over every permission bit again in
> the table/sequence grant permission code, but the union allowed for a
> very simple check in that code.
>
> It allows for better code checks and I think it behaves as expected:
>
> test=> CREATE TABLE tab(x INTEGER);
> CREATE TABLE
> test=> CREATE SEQUENCE seq;
> CREATE SEQUENCE
> test=> GRANT ALL ON seq, tab TO PUBLIC;
> GRANT
> test=> REVOKE USAGE ON seq, tab FROM PUBLIC;
> ERROR: invalid privilege type USAGE for table
> test=> REVOKE SELECT ON seq, tab FROM PUBLIC;
> REVOKE
> test=> REVOKE DELETE ON seq, tab FROM PUBLIC;
> WARNING: sequence "seq" only supports USAGE, SELECT, AND UPDATE
> WARNING: no privileges could be revoked for "seq"
> REVOKE
>
> and pg_dump has:
>
> GRANT USAGE,UPDATE ON SEQUENCE x TO PUBLIC;
>
> GRANT INSERT,RULE,UPDATE,REFERENCES,TRIGGER ON TABLE xx TO PUBLIC;
>
> Note I had to add the object name to the warning message so it is clear
> which object permission changes did succeed. I have updated the patch.
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073