Prev: Protocol 3, Execute, maxrows to return, impact?
Next: PATCH: CITEXT 2.0 v3
From: Abhijit Menon-Sen on 9 Jul 2008 14:58
At 2008-07-08 09:32:44 -0400, alvherre(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Alvaro Herrera on 9 Jul 2008 15:11
Abhijit Menon-Sen escribi�:
> At 2008-07-08 09:32:44 -0400, alvherre(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Abhijit Menon-Sen on 9 Jul 2008 23:11
At 2008-07-09 15:11:25 -0400, alvherre(a)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(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Abhijit Menon-Sen on 12 Jul 2008 07:30
At 2008-07-11 11:57:37 -0500, jcasanov(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Abhijit Menon-Sen on 12 Jul 2008 15:52
At 2008-07-12 14:32:03 -0500, jcasanov(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2 3
Prev: Protocol 3, Execute, maxrows to return, impact?
Next: PATCH: CITEXT 2.0 v3