Re: [HACKERS] Inconsistent syntax in GRANT - Mailing list pgsql-patches

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.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@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073