Re: Extending grant insert on tables to sequences

Hi,

The idea of this patch is to avoid the need to make explicit grants on
sequences owned by tables.

This patch make:
- GRANT INSERT ON TABLE extend to GRANT USAGE ON SEQUENCE (currval, nextval)
- GRANT UPDATE ON TABLE extend to GRANT UPDATE ON SEQUENCE (nextval, setval)
- GRANT SELECT ON TABLE extend to GRANT SELECT ON SEQUENCE (currval)

comments?

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 087171157

On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards(at)gmail(dot)com> wrote:
> Hi,
>
> The idea of this patch is to avoid the need to make explicit grants on
> sequences owned by tables.
>

I've noted that the patch i attached is an older version that doesn't
compile because of a typo...
Re-attaching right patch and fix documentation to indicate the new behaviour...

we need an user visible message to indicate this implicit grant on the
sequences?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 087171157

Jaime Casanova escribió:
> On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards(at)gmail(dot)com> wrote:
> > Hi,
> >
> > The idea of this patch is to avoid the need to make explicit grants on
> > sequences owned by tables.
> >
>
> I've noted that the patch i attached is an older version that doesn't
> compile because of a typo...
> Re-attaching right patch and fix documentation to indicate the new behaviour...

Please add the patch to the commitfest page,

http://wiki.postgresql.org/wiki/CommitFest:July

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Sat, May 24, 2008 at 12:09 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
>
> Please add the patch to the commitfest page,
>

Ah! I forgot we have a new process now... patch added to the commitfest page...

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 087171157

On Saturday 24 May 2008 01:19:05 Jaime Casanova wrote:
> On Sat, May 24, 2008 at 12:09 AM, Alvaro Herrera
>
> <alvherre(at)commandprompt(dot)com> wrote:
> > Please add the patch to the commitfest page,
>
> Ah! I forgot we have a new process now... patch added to the commitfest
> page...
>

What's the use case for extending SELECT on table to SELECT on sequence ?

--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL

On 5/29/08, Robert Treat <xzilla(at)users(dot)sourceforge(dot)net> wrote:
> On Saturday 24 May 2008 01:19:05 Jaime Casanova wrote:
> > On Sat, May 24, 2008 at 12:09 AM, Alvaro Herrera
> >
> > <alvherre(at)commandprompt(dot)com> wrote:
> > > Please add the patch to the commitfest page,
> >
> > Ah! I forgot we have a new process now... patch added to the commitfest
> > page...
> >
>
> What's the use case for extending SELECT on table to SELECT on sequence ?
>

Just to be consistent

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 087171157

Jaime Casanova escribió:
> On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards(at)gmail(dot)com> wrote:
> > Hi,
> >
> > The idea of this patch is to avoid the need to make explicit grants on
> > sequences owned by tables.
>
> I've noted that the patch i attached is an older version that doesn't
> compile because of a typo...
> Re-attaching right patch and fix documentation to indicate the new behaviour...

I had a look at this patch and it looks good. The only thing that's not
clear to me is whether we have agreed we want this to be the default
behavior?

A quibble:

> + foreach(cell, istmt.objects)
> + {
> + [...]
> +
> + istmt_seq.objects = getOwnedSequences(lfirst_oid(cell));
> + if (istmt_seq.objects != NIL)
> + {
> + if (istmt.privileges & (ACL_INSERT))
> + istmt_seq.privileges |= ACL_USAGE;
> + else if (istmt.privileges & (ACL_UPDATE))
> + istmt_seq.privileges |= ACL_UPDATE;
> + else if (istmt.privileges & (ACL_SELECT))
> + istmt_seq.privileges |= ACL_SELECT;
> +
> + ExecGrantStmt_oids(&istmt_seq);
> + }

Wouldn't it be clearer to build a list with all the sequences owned by
the tables in istmt.objects, and then call ExecGrantStmt_oids() a single
time with the big list?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 7/8/08, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Jaime Casanova escribió:
> > On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards(at)gmail(dot)com> wrote:
> > > Hi,
> > >
> > > The idea of this patch is to avoid the need to make explicit grants on
> > > sequences owned by tables.
> >
> > I've noted that the patch i attached is an older version that doesn't
> > compile because of a typo...
> > Re-attaching right patch and fix documentation to indicate the new behaviour...
>
> I had a look at this patch and it looks good. The only thing that's not
> clear to me is whether we have agreed we want this to be the default
> behavior?
>

mmm... i don't remember from where i took the equivalences...
i will review if there is any concensus in that...
anyway now i when people should speak about it...

>
> Wouldn't it be clearer to build a list with all the sequences owned by
> the tables in istmt.objects, and then call ExecGrantStmt_oids() a single
> time with the big list?
>

at night i will see the code for this...

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157

At 2008-07-08 09:32:44 -0400, alvherre(at)commandprompt(dot)com wrote:
>
> > > The idea of this patch is to avoid the need to make explicit
> > > grants on sequences owned by tables. [...]
>
> I had a look at this patch and it looks good. The only thing that's
> not clear to me is whether we have agreed we want this to be the
> default behavior?

For what it's worth, I quite like the idea.

(I looked at the patch, and it looks good to me too.)

> Wouldn't it be clearer to build a list with all the sequences owned by
> the tables in istmt.objects, and then call ExecGrantStmt_oids() a
> single time with the big list?

i.e., to hoist most of the istmt_seq initialisation out of the loop,
right? Yes, that makes sense.

-- ams

Abhijit Menon-Sen escribió:
> At 2008-07-08 09:32:44 -0400, alvherre(at)commandprompt(dot)com wrote:

> > Wouldn't it be clearer to build a list with all the sequences owned by
> > the tables in istmt.objects, and then call ExecGrantStmt_oids() a
> > single time with the big list?
>
> i.e., to hoist most of the istmt_seq initialisation out of the loop,
> right? Yes, that makes sense.

No, actually I meant having a lone "list = lappend(list, newseq);" in
the loop, so that ExecGrantStmt_oids is called only once. The
initialization is done only once too, of course.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

At 2008-07-09 15:11:25 -0400, alvherre(at)commandprompt(dot)com wrote:
>
> No, actually I meant having a lone "list = lappend(list, newseq);" in
> the loop, so that ExecGrantStmt_oids is called only once.

Yes, I understand what you meant. I just phrased my agreement poorly.
Here's a more precise phrasing. ;-)

(I agree with Robert Treat that there seems to be no point granting
SELECT on the sequence. I don't *particularly* care about it, but I
tend towards wanting to drop that bit. This patch reflects that.)

Jaime: please feel free to use or ignore this, as you wish.

-- ams

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 15f5af0..8664203 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -361,6 +361,41 @@ ExecuteGrantStmt(GrantStmt *stmt)
}

ExecGrantStmt_oids(&istmt);
+
+ /* If INSERT or UPDATE privileges are being granted or revoked on a
+ * relation, this extends the operation to include any sequences
+ * owned by the relation.
+ */
+
+ if (istmt.objtype == ACL_OBJECT_RELATION &&
+ (istmt.privileges & (ACL_INSERT | ACL_UPDATE)))
+ {
+ InternalGrant istmt_seq;
+
+ istmt_seq.is_grant = istmt.is_grant;
+ istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
+ istmt_seq.grantees = istmt.grantees;
+ istmt_seq.grant_option = istmt.grant_option;
+ istmt_seq.behavior = istmt.behavior;
+ istmt_seq.all_privs = false;
+
+ istmt_seq.privileges = ACL_NO_RIGHTS;
+ if (istmt.privileges & ACL_INSERT)
+ istmt_seq.privileges |= ACL_USAGE;
+ if (istmt.privileges & ACL_UPDATE)
+ istmt_seq.privileges |= ACL_UPDATE;
+
+ istmt_seq.objects = NIL;
+ foreach (cell, istmt.objects)
+ {
+ istmt_seq.objects =
+ list_concat(istmt_seq.objects,
+ getOwnedSequences(lfirst_oid(cell)));
+ }
+
+ if (istmt_seq.objects != NIL)
+ ExecGrantStmt_oids(&istmt_seq);
+ }
}

/*

On Wed, Jul 9, 2008 at 10:11 PM, Abhijit Menon-Sen <ams(at)oryx(dot)com> wrote:
> At 2008-07-09 15:11:25 -0400, alvherre(at)commandprompt(dot)com wrote:
>>
>> No, actually I meant having a lone "list = lappend(list, newseq);" in
>> the loop, so that ExecGrantStmt_oids is called only once.
>
> Yes, I understand what you meant. I just phrased my agreement poorly.
> Here's a more precise phrasing. ;-)
>
> (I agree with Robert Treat that there seems to be no point granting
> SELECT on the sequence. I don't *particularly* care about it, but I
> tend towards wanting to drop that bit. This patch reflects that.)
>

Hi,
sorry for the delay i was busy...

attached is a new version of the patch, it implements Alvaro's
suggestion and fix a bug i found (it wasn't managing GRANT ALL) :(

About the SELECT issue, AFAIU Robert doesn't complaint he just asked
what is the use case... if people think it should be removed ok, but
OTOH: why? i don't think that affects anything...

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157

At 2008-07-11 11:57:37 -0500, jcasanov(at)systemguards(dot)com(dot)ec wrote:
>
> attached is a new version of the patch, it implements Alvaro's
> suggestion and fix a bug i found (it wasn't managing GRANT ALL) :(

Looks good to me.

> About the SELECT issue, AFAIU Robert doesn't complaint he just asked
> what is the use case... if people think it should be removed ok, but
> OTOH: why? i don't think that affects anything...

As I said, I don't feel too strongly about it.

> <para>
> ! Granting permission on a table automatically extend
> ! permissions to any sequences owned by the table, including
> ! sequences tied to <type>SERIAL</> columns.
> </para>

Should be "Granting permissions on a table automatically extends those
permissions to...".

> + if ((istmt.objtype == ACL_OBJECT_RELATION) && (istmt.all_privs ||
> + (istmt.privileges & (ACL_INSERT | ACL_UPDATE | ACL_SELECT))))
> + {

The parentheses around the first comparison can go away, and also the
ones around the ACL_* here:

> + if (istmt.privileges & (ACL_INSERT))
> + istmt_seq.privileges |= ACL_USAGE;
> + if (istmt.privileges & (ACL_UPDATE))
> + istmt_seq.privileges |= ACL_UPDATE;
> + if (istmt.privileges & (ACL_SELECT))
> + istmt_seq.privileges |= ACL_SELECT;

-- ams

On Sat, Jul 12, 2008 at 6:30 AM, Abhijit Menon-Sen <ams(at)oryx(dot)com> wrote:
>
>> <para>
>> ! Granting permission on a table automatically extend
>> ! permissions to any sequences owned by the table, including
>> ! sequences tied to <type>SERIAL</> columns.
>> </para>
>
> Should be "Granting permissions on a table automatically extends those
> permissions to...".
>

what about "extends them to..."

>> + if ((istmt.objtype == ACL_OBJECT_RELATION) && (istmt.all_privs ||
>> + (istmt.privileges & (ACL_INSERT | ACL_UPDATE | ACL_SELECT))))
>> + {
>
> The parentheses around the first comparison can go away, and also the
> ones around the ACL_* here:
>

ok

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157

At 2008-07-12 14:32:03 -0500, jcasanov(at)systemguards(dot)com(dot)ec wrote:
>
> > Should be "Granting permissions on a table automatically extends
> > those permissions to...".
>
> what about "extends them to..."

Yes, sounds fine, thanks.

But I notice that nobody else has commented on whether they want this
feature or not. Does anyone particularly dislike the idea?

-- ams

"Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec> writes:
> + if (istmt.all_privs)
> + istmt_seq.all_privs = true;
> + else
> + {
> + if (istmt.privileges & ACL_INSERT)
> + istmt_seq.privileges |= ACL_USAGE;
> + if (istmt.privileges & ACL_UPDATE)
> + istmt_seq.privileges |= ACL_UPDATE;
> + if (istmt.privileges & ACL_SELECT)
> + istmt_seq.privileges |= ACL_SELECT;
> + }

This definition of the derived rights seems pretty arbitrary and
unprincipled. If you can't explain it precisely in a few words in the
documentation (and I notice you didn't even attempt to explain it at
all), then you probably need to think harder.

In particular, I don't see why having UPDATE on the parent table should
grant the right to use setval() on the sequence. If you had INSERT and
DELETE as well, implying the right to make arbitrary changes in the
parent table, then maybe setval() would be sensible to allow --- but
this code can't really tell that, since it doesn't have a global view
of the privileges previously granted.

What I think makes sense is just to have parent INSERT privilege lead to
USAGE on the sequence (thus granting nextval/currval rights, which are
what you'd typically need in association with trying to do inserts).
I don't see any need to automatically grant more than that. Selects and
updates on the parent table don't need to touch the sequence, so why are
those privileges granting anything?

regards, tom lane

Abhijit Menon-Sen <ams(at)oryx(dot)com> writes:
> But I notice that nobody else has commented on whether they want this
> feature or not. Does anyone particularly dislike the idea?

I think it's probably reasonable as long as we keep the implicitly
granted rights as narrow as possible. INSERT on the parent table
would normally be hard to use correctly if you can't nextval() the
sequence, so automatically allowing nextval() seems pretty reasonable.
I think the case for granting anything more than that is weak ---
even without considering backwards-compatibility arguments.

A fairly important practical problem is whether this will keep pg_dump
from correctly reproducing the state of a database. Assume that someone
did revoke the implicitly-granted rights on the sequence --- would a
dump and reload correctly preserve that state? It'd depend on the order
in which pg_dump issued the GRANTs, and I'm not at all sure pg_dump
could be relied on to get that right. (Even if we fixed it to account
for the issue today, what of older dump scripts?)

Another issue is the interaction with the planned column-level GRANT
feature. AFAICS, the obvious-sounding rule that usage of the sequence
should be granted consequent to granting INSERT on the owning column
would be exactly backwards. It's when you have *not* got INSERT on
that column that you *must* rely on the default for it, and hence you'd
better have the ability to do nextval() or your alleged insert
privileges on other columns are worthless. So it seems that sequence
usage should be granted if any column INSERT is granted, and revoked
only when all column INSERT privileges are revoked --- and that latter
rule is going to be hard to implement with this type of patch, because
it doesn't know what column privileges are going to remain.

I thought for a bit about abandoning the proposed implementation and
instead having nextval/currval check at runtime: IOW, if the check for
ACL_USAGE on the sequence fails, look to see if the sequence is "owned"
and if so look to see if the user has ACL_INSERT on the parent table.
(This seems a bit slow but maybe it wouldn't be a problem, or maybe we
could arrange to cache the lookup results.) This would avoid the
"action at a distance" behavior in GRANT and thereby cure both of
the problems mentioned above. However, it would mean that it'd be
impossible to grant INSERT without effectively granting sequence USAGE
--- revoking USAGE on the sequence wouldn't stop anything. Plus, \z on
the sequence would fail to tell you about those implicitly held rights.
So I'm not sure I like this way any better.

regards, tom lane

Sorry for the delay in the answer but i was busy with 2 projects and a talk...

On Sat, Jul 12, 2008 at 3:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I think it's probably reasonable as long as we keep the implicitly
> granted rights as narrow as possible. INSERT on the parent table
> would normally be hard to use correctly if you can't nextval() the
> sequence, so automatically allowing nextval() seems pretty reasonable.
> I think the case for granting anything more than that is weak ---
> even without considering backwards-compatibility arguments.
>

ok. at least two more reviewers make questions against the SELECT permission...
my point was that if keep INSERT and UPDATE permissions then keep
SELECT as well... but let *only* INSERT's seems enough to me...

> A fairly important practical problem is whether this will keep pg_dump
> from correctly reproducing the state of a database. Assume that someone
> did revoke the implicitly-granted rights on the sequence --- would a
> dump and reload correctly preserve that state? It'd depend on the order
> in which pg_dump issued the GRANTs, and I'm not at all sure pg_dump
> could be relied on to get that right. (Even if we fixed it to account
> for the issue today, what of older dump scripts?)
>

good point! a simple test make me think that yes, i will try some
complex cases to be sure (actually i think it should be a problem
here)

> Another issue is the interaction with the planned column-level GRANT
> feature.
>

Although that is a feature we want, is a WIP one... do we stop patches
because it can conflict with a project we don't know will be applied
soon?

In any case, i will review that patch to see where we are on that and
to try make those two compatible...

>
> I thought for a bit about abandoning the proposed implementation and
> instead having nextval/currval check at runtime: IOW, if the check for
> ACL_USAGE on the sequence fails, look to see if the sequence is "owned"
> and if so look to see if the user has ACL_INSERT on the parent table.
> (This seems a bit slow but maybe it wouldn't be a problem, or maybe we
> could arrange to cache the lookup results.) This would avoid the
> "action at a distance" behavior in GRANT and thereby cure both of
> the problems mentioned above. However, it would mean that it'd be
> impossible to grant INSERT without effectively granting sequence USAGE
> --- revoking USAGE on the sequence wouldn't stop anything. Plus, \z on
> the sequence would fail to tell you about those implicitly held rights.

seems like a hackish... do we want this? comments?

i will work on this patch for the next days...

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157

"Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec> writes:
>> Another issue is the interaction with the planned column-level GRANT
>> feature.

> Although that is a feature we want, is a WIP one... do we stop patches
> because it can conflict with a project we don't know will be applied
> soon?

Well, considering that that one is implementing a feature required by
SQL spec, your feature will lose any tug-of-war ;-). So yeah, you
ought to consider how to make yours play nice when (not if) that
happens.

regards, tom lane

Added to September commit fest.

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

Abhijit Menon-Sen wrote:
> At 2008-07-09 15:11:25 -0400, alvherre(at)commandprompt(dot)com wrote:
> >
> > No, actually I meant having a lone "list = lappend(list, newseq);" in
> > the loop, so that ExecGrantStmt_oids is called only once.
>
> Yes, I understand what you meant. I just phrased my agreement poorly.
> Here's a more precise phrasing. ;-)
>
> (I agree with Robert Treat that there seems to be no point granting
> SELECT on the sequence. I don't *particularly* care about it, but I
> tend towards wanting to drop that bit. This patch reflects that.)
>
> Jaime: please feel free to use or ignore this, as you wish.
>
> -- ams
>
> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
> index 15f5af0..8664203 100644
> --- a/src/backend/catalog/aclchk.c
> +++ b/src/backend/catalog/aclchk.c
> @@ -361,6 +361,41 @@ ExecuteGrantStmt(GrantStmt *stmt)
> }
>
> ExecGrantStmt_oids(&istmt);
> +
> + /* If INSERT or UPDATE privileges are being granted or revoked on a
> + * relation, this extends the operation to include any sequences
> + * owned by the relation.
> + */
> +
> + if (istmt.objtype == ACL_OBJECT_RELATION &&
> + (istmt.privileges & (ACL_INSERT | ACL_UPDATE)))
> + {
> + InternalGrant istmt_seq;
> +
> + istmt_seq.is_grant = istmt.is_grant;
> + istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
> + istmt_seq.grantees = istmt.grantees;
> + istmt_seq.grant_option = istmt.grant_option;
> + istmt_seq.behavior = istmt.behavior;
> + istmt_seq.all_privs = false;
> +
> + istmt_seq.privileges = ACL_NO_RIGHTS;
> + if (istmt.privileges & ACL_INSERT)
> + istmt_seq.privileges |= ACL_USAGE;
> + if (istmt.privileges & ACL_UPDATE)
> + istmt_seq.privileges |= ACL_UPDATE;
> +
> + istmt_seq.objects = NIL;
> + foreach (cell, istmt.objects)
> + {
> + istmt_seq.objects =
> + list_concat(istmt_seq.objects,
> + getOwnedSequences(lfirst_oid(cell)));
> + }
> +
> + if (istmt_seq.objects != NIL)
> + ExecGrantStmt_oids(&istmt_seq);
> + }
> }
>
> /*
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Fri, Aug 22, 2008 at 10:19 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> Added to September commit fest.
>

why? there isn't a new patch yet... i haven't sent it because i want
to see the column level privileges patch first because Tom's
complaints

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. (593) 87171157

On Fri, Aug 22, 2008 at 10:19 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> Added to September commit fest.
>

updating the patch with one that only extends inserts. though, i
haven't look at the col level privs patch yet.

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. (593) 87171157

* Jaime Casanova (jcasanov(at)systemguards(dot)com(dot)ec) wrote:
> On Fri, Aug 22, 2008 at 10:19 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >
> > Added to September commit fest.
> >
>
> updating the patch with one that only extends inserts. though, i
> haven't look at the col level privs patch yet.

At least initially I wasn't planning to support column-level privileges
for sequences, so I don't think it will affect you much. Do people
think it makes sense to try and support that?

As your patch appears more ready-for-commit than the column-level
privileges patch, I wouldn't worry about what code might have to move
around, that'll be for me to deal with in a re-sync with HEAD once your
patch is committed.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Jaime Casanova (jcasanov(at)systemguards(dot)com(dot)ec) wrote:
>> updating the patch with one that only extends inserts. though, i
>> haven't look at the col level privs patch yet.

> At least initially I wasn't planning to support column-level privileges
> for sequences, so I don't think it will affect you much. Do people
> think it makes sense to try and support that?

USAGE certainly wouldn't be column-level in any case --- it'd be a
privilege on the sequence as such. That end of it isn't the problem;
the problem is that column-level privileges on the table make it hard to
decide when to grant rights on the sequence, as I pointed out last time
round:
http://archives.postgresql.org/pgsql-hackers/2008-07/msg00624.php

> As your patch appears more ready-for-commit than the column-level
> privileges patch, I wouldn't worry about what code might have to move
> around, that'll be for me to deal with in a re-sync with HEAD once your
> patch is committed.

I think that's backwards. The above message raises serious concerns
about whether the USAGE-granting patch can be implemented at all in the
presence of column-level privileges. I think the right thing is to get
column privileges in and then see if it's possible to implement
USAGE-granting compatibly. I don't want to commit a patch that is
clearly going to be broken when (not if) column privileges arrive.

I note also that no response was given to my worries about pg_dump
behavior.

In short, this patch isn't much more ready to commit than it was
in the last fest.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Jaime Casanova (jcasanov(at)systemguards(dot)com(dot)ec) wrote:
> >> updating the patch with one that only extends inserts. though, i
> >> haven't look at the col level privs patch yet.
>
> > At least initially I wasn't planning to support column-level privileges
> > for sequences, so I don't think it will affect you much. Do people
> > think it makes sense to try and support that?
>
> USAGE certainly wouldn't be column-level in any case --- it'd be a
> privilege on the sequence as such. That end of it isn't the problem;
> the problem is that column-level privileges on the table make it hard to
> decide when to grant rights on the sequence, as I pointed out last time
> round:
> http://archives.postgresql.org/pgsql-hackers/2008-07/msg00624.php

Ah, obviously I hadn't read far enough back about this patch. I agree
that sequence USAGE should be granted when insert is granted on any
column. One suggestion is that as the SQL spec indicates that a
table-level revoke implies a revoke on all columns, we could have the
revokation of the sequence permissisons done only on table-level
revokation of insert and not on any individual column-level insert, even
if that was the last column which insert rights were granted on.

I have to admit that I'm not a big fan of that though because a given
state on the table wouldn't imply a particular state for the sequence-
it would depend on how you got there. The way the code is currently
laid out for the column-level privileges, it wouldn't be that difficult
to go through all of the other columns and check if this was the last
insert being revoked, but I don't particularly like that either, and
it strikes me as 99% of the time being wasted effort. I guess if we
could check for and only go through that effort when there is a sequence
in place with implicit grants it might not be too bad.

> > As your patch appears more ready-for-commit than the column-level
> > privileges patch, I wouldn't worry about what code might have to move
> > around, that'll be for me to deal with in a re-sync with HEAD once your
> > patch is committed.
>
> I think that's backwards. The above message raises serious concerns
> about whether the USAGE-granting patch can be implemented at all in the
> presence of column-level privileges. I think the right thing is to get
> column privileges in and then see if it's possible to implement
> USAGE-granting compatibly. I don't want to commit a patch that is
> clearly going to be broken when (not if) column privileges arrive.

Now that I understand the situation better, I agree with you on this. I
hadn't realized this patch was about implicit grants on sequnces. Sorry
for the noise.

Thanks,

Stephen

On Wed, Sep 3, 2008 at 7:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> In short, this patch isn't much more ready to commit than it was
> in the last fest.
>

Just for the record, i put this updated patch just because there were
an entry for "Extending grant insert on tables to sequences" for this
Commit Fest without being an updated patch

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. (593) 87171157