Thread: Re: [PATCHES] Extending grant insert on tables to sequences
Jaime Casanova escribió:
> On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards@gmail.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@commandprompt.com> wrote: > Jaime Casanova escribió: > > On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards@gmail.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@commandprompt.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@commandprompt.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@commandprompt.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@oryx.com> wrote: > At 2008-07-09 15:11:25 -0400, alvherre@commandprompt.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
Attachment
At 2008-07-11 11:57:37 -0500, jcasanov@systemguards.com.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@oryx.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
Attachment
At 2008-07-12 14:32:03 -0500, jcasanov@systemguards.com.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@systemguards.com.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@oryx.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@sss.pgh.pa.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@systemguards.com.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@commandprompt.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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
-- Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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
Attachment
* Jaime Casanova (jcasanov@systemguards.com.ec) wrote:
> On Fri, Aug 22, 2008 at 10:19 PM, Bruce Momjian <bruce@momjian.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@snowman.net> writes: > * Jaime Casanova (jcasanov@systemguards.com.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@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > * Jaime Casanova (jcasanov@systemguards.com.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@sss.pgh.pa.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