Re: [HACKERS] Inconsistent syntax in GRANT

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