On Thu, Jan 15, 2009 at 12:33 AM, KaiGai Kohei kai...@ak.jp.nec.com wrote:
It seems to me ExecGrant_Relation() is a bit larger than other
ExecGrant_()s.
My preference is to clip out column-privilege part into ExecGrant_Attribute()
and invoke it for each given columns.
But, it is just my
Jaime,
* Jaime Casanova (jcasa...@systemguards.com.ec) wrote:
while i'm not an official commiter/reviewer, it seems natural to me to
have an ExecGrant_Attribute() function.
Thanks for the comment. I've gone ahead and done this now, and I do
think it improves the code overall and tells a
KaiGai,
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
The attached patch put invocations of markColumnForSelectPriv()
at transformJoinUsingClause() to mark those columns are used.
Thanks for the update!
Attached is a patch which:
- incorporates KaiGai's latest patches to deal with JOINs and
Stephen, I could find a strange behavior unfortunatelly. :-)
(But it is obvious one, don't worry.)
postgres=# CREATE TABLE t1 (a int, b int, c int);
CREATE TABLE
postgres=# ALTER TABLE t1 DROP COLUMN c;
ALTER TABLE
postgres=# \c - ymj
psql (8.4devel)
You are now connected to
KaiGai,
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
Stephen, I could find a strange behavior unfortunatelly. :-)
Glad you're finding these, honestly. Better to find them and deal with
them now than after a release.
It is a case to be failed because 'ymj' has no privileges on t1
and its
BTW, what is the official reviewer's opinion?
It seems to me most of the issues on column-level privileges are
resolved, so it is almost ready for getting merged.
I tend to doubt Tom's had a chance to review it again yet, which is
fine, though I'm certainly hopeful the recent focus and our
The attached patch is proof of the concept.
It can be applied on the latest CVS HEAD with colprivs_2009011001.diff.gz.
- It unpatches unnecessary updates at parser/parse_expr.c, parser/parse_node.c
and parser/parse_relation.c.
- It adds markColumnForSelectPriv() to mark proper rte-cols_sel for
Tom, er al,
* Tom Lane (t...@sss.pgh.pa.us) wrote:
I'm thinking make_var is not the place to do this. The places that are
supposed to be taking care of permissions are the ones that do this:
/* Require read access --- see comments in setTargetTable() */
KaiGai,
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
The attached patch is proof of the concept.
Thanks!
It can be applied on the latest CVS HEAD with colprivs_2009011001.diff.gz.
- It unpatches unnecessary updates at parser/parse_expr.c, parser/parse_node.c
and parser/parse_relation.c.
Stephen Frost sfr...@snowman.net writes:
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
- The markColumnForSelectPriv() uses walker function internally, because
there is no guarantee all the entities within RangeTblEntry-joinaliasvars
are Var type node.
If any of them aren't Vars, then
Stephen Frost sfr...@snowman.net writes:
It's possible that we've missed some --- in particular, right at the
moment I am not sure that whole-row Vars are handled properly.
I added specific regression test for whole-row Vars, so I'd be concerned
if something isn't working there.
What I see
* Tom Lane (t...@sss.pgh.pa.us) wrote:
What I see being tested is SELECT *, which is a different animal
entirely. As required by spec, SELECT * is expanded to a list of
ordinary variables at parse time and then it's not really a special
case anymore. A true whole-row variable only occurs
Stephen Frost sfr...@snowman.net writes:
* Tom Lane (t...@sss.pgh.pa.us) wrote:
What I see being tested is SELECT *, which is a different animal
entirely.
Wouldn't this test cover those?
SELECT atest5 FROM atest5; -- fail
Oh, I didn't see that. Still, this doesn't test whether the
* Tom Lane (t...@sss.pgh.pa.us) wrote:
Stephen Frost sfr...@snowman.net writes:
* Tom Lane (t...@sss.pgh.pa.us) wrote:
What I see being tested is SELECT *, which is a different animal
entirely.
Wouldn't this test cover those?
SELECT atest5 FROM atest5; -- fail
Oh, I didn't see
Stephen Frost sfr...@snowman.net writes:
* Tom Lane (t...@sss.pgh.pa.us) wrote:
Oh, I didn't see that. Still, this doesn't test whether the behavior
is correct with respect to ADD/DROP COLUMN --- if that were implemented
like SELECT * you'd not see any change in the regression result.
Hrm.
Stephen Frost wrote:
Tom, er al,
* Tom Lane (t...@sss.pgh.pa.us) wrote:
I'm thinking make_var is not the place to do this. The places that are
supposed to be taking care of permissions are the ones that do this:
/* Require read access --- see comments in setTargetTable() */
KaiGai,
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
It seems to me you didn't add success cases for JOINs.
The previous patch tries to check privilege for each columns within
JOIN'ed tables unexpectedly, so the test case always fails.
Right, I realized that when I went back and looked at it.
Tom Lane wrote:
BTW, another corner case that I'm not sure gets handled right is
that the join columns in JOIN USING or NATURAL JOIN need to be marked
as requiring ACL_SELECT. (Or so I'd expect anyway; I didn't run through
the SQL spec looking for chapter and verse on that.) I forget whether
Stephen,
The revised patch can reproduce the original matter, as follows:
postgres=# CREATE TABLE t1 (a int, b text);
CREATE TABLE
postgres=# CREATE TABLE t2 (x int, y text);
CREATE TABLE
postgres=# GRANT select(a) on t1 to ymj;
GRANT
postgres=# GRANT select(x,y) on t2 TO ymj;
I can find a matter on JOIN.
Please make sure the following function invocation chain:
transformSelectStmt()
- transformFromClause()
- transformFromClauseItem()
- expandRTE(), if JoinExpr
- expandRelation(), if rte-rtekind == RTE_RELATION
- expandTupleDesc()
expandTupleDesc() set
KaiGai Kohei kai...@ak.jp.nec.com writes:
I reconsidered the previous walker implementation independent
from other parser codes is more simple and better.
And slower, and equally subject to this bug, so I'm not convinced.
Stephen, Tom, what is your opinion?
I'm thinking make_var is not the
Tom Lane wrote:
I'm thinking make_var is not the place to do this. The places that are
supposed to be taking care of permissions are the ones that do this:
/* Require read access --- see comments in setTargetTable() */
rte-requiredPerms |= ACL_SELECT;
Indeed, it
Tom, et al,
* Stephen Frost (sfr...@snowman.net) wrote:
applyColumnPrivs is misnamed as it's not applying any privileges nor
indeed doing much of anything directly to do with privileges. It should
probably be named something more like findReferencedColumns. And what's
with the special
Jamie, et al,
* Jaime Casanova (jcasa...@systemguards.com.ec) wrote:
para
! Currently, productnamePostgreSQL/productname does not recognize
! column-level SELECT privileges when a JOIN is involved.
One possible workaround is to create a view having just the desired
Stephen Frost sfr...@snowman.net writes:
Please test, comment, etc.
A few random comments based on a very fast scan through the patch:
The RTE fields really ought to be bitmaps not integer lists. The
list representation is expensive to store, copy, etc. You could use
the same approach the HOT
Tom, et al,
* Tom Lane (t...@sss.pgh.pa.us) wrote:
A few random comments based on a very fast scan through the patch:
Thanks for the feedback!
The RTE fields really ought to be bitmaps not integer lists. The
list representation is expensive to store, copy, etc. You could use
the same
KaiGai Kohei kai...@ak.jp.nec.com writes:
ExecCheckRTEPerms() checks user's privileges on columns, when he does
not have required privileges on the table. When he has proper privileges
on all the appeared columns within the table, it is allowed.
But, when no columns are used on the table, it
Tom, et al,
* Tom Lane (t...@sss.pgh.pa.us) wrote:
KaiGai Kohei kai...@ak.jp.nec.com writes:
ExecCheckRTEPerms() checks user's privileges on columns, when he does
not have required privileges on the table. When he has proper privileges
on all the appeared columns within the table, it is
Stephen Frost sfr...@snowman.net writes:
I'm open to suggestions about how to handle this. My first thought
would be- add an entry to the cols_sel list for the RTE that is special
and indicates any column, perhaps by using a '0' for the attrid, as is
done elsewhere. Then modify
* Tom Lane (t...@sss.pgh.pa.us) wrote:
Stephen Frost sfr...@snowman.net writes:
I'm open to suggestions about how to handle this. My first thought
would be- add an entry to the cols_sel list for the RTE that is special
and indicates any column, perhaps by using a '0' for the attrid, as is
Stephen Frost wrote:
* Tom Lane (t...@sss.pgh.pa.us) wrote:
Stephen Frost sfr...@snowman.net writes:
I'm open to suggestions about how to handle this. My first thought
would be- add an entry to the cols_sel list for the RTE that is special
and indicates any column, perhaps by using a '0' for
KaiGai,
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
I've thought about it some, and yes, that sounds reasonable. I'll try
and implement it tonight and test it out.
I've implemented this and it appears to work well.
When we refer table-rowtype, analyzer handles its Var-varattno has
On Thu, Jan 8, 2009 at 11:34 PM, Stephen Frost sfr...@snowman.net wrote:
Please let me know if you have comments.
a fast test seems to show that this documentation change is no longer
true ;) nice work!
--- 443,450
/para
para
! Currently, productnamePostgreSQL/productname
KaiGai, Tom, all,
Attached is an updated patch which fixes Markus' dependency issue,
handles the other issues mentioned by KaiGai and Tom, has some
additional regressions tests to make sure all of that works, and has
been cleaned up to the 80-col goal (where it seemed to work well and
Hi,
KaiGai Kohei wrote:
The attached patch is a proof of the concept.
Awesome! I'll try to review during the day.
I strongly want the Column-level privileges to be get merged
as soon as possible, so I don't spare any possible assist
for his works.
+1
Can you quickly comment on CLP vs.
Markus Wanner wrote:
Hi,
KaiGai Kohei wrote:
The attached patch is a proof of the concept.
Awesome! I'll try to review during the day.
I strongly want the Column-level privileges to be get merged
as soon as possible, so I don't spare any possible assist
for his works.
+1
Can you quickly
KaiGai,
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
Is it possible to implement a walker function to pick up appeared
columns and to chain them on rte-cols_sel/cols_mod?
In this idea, columns in Query-targetList should be chained on
rte-cols_mod, and others should be chained on rte-cols_sel.
Stephen Frost wrote:
KaiGai,
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
Is it possible to implement a walker function to pick up appeared
columns and to chain them on rte-cols_sel/cols_mod?
In this idea, columns in Query-targetList should be chained on
rte-cols_mod, and others should be
KaiGai,
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
In addition, please note that expression_tree_walker() invokes
check_stack_depth() to prevent unexpected stack overflow.
Ah, that's the part I was looking for and somehow overlooked. As long
as we're checking at some point then I worry alot
On Wed, Jan 7, 2009 at 1:46 AM, KaiGai Kohei kai...@ak.jp.nec.com wrote:
The attached patch is a proof of the concept.
It walks on a given query tree to append accessed columns on
rte-cols_sel and rte-cols_mod.
When aliasvar of JOIN'ed relation is accesses, its source is
appended on the
Jaime Casanova wrote:
On Wed, Jan 7, 2009 at 1:46 AM, KaiGai Kohei kai...@ak.jp.nec.com wrote:
The attached patch is a proof of the concept.
It walks on a given query tree to append accessed columns on
rte-cols_sel and rte-cols_mod.
When aliasvar of JOIN'ed relation is accesses, its source is
Markus Wanner wrote:
Hello Stephen,
Stephen Frost wrote:
..in the attached patch.
Thanks, I've reviewed this patch again.
Please find attached an updated patch for column-level privileges
which incorporates Alvaro's suggested changes and is updated to the
latest CVS HEAD.
Cool, applies
Currently,
column-level privileges are not honored when JOINs are involved (you
must have the necessary table-level privileges, as you do today). It
would really be great to have that working and mainly involves
modifying the rewriter to add on to the appropriate range table column
list entries
Markus,
* Markus Wanner (mar...@bluegap.ch) wrote:
Stephen Frost wrote:
..in the attached patch.
Thanks, I've reviewed this patch again.
Thanks!
Currently,
column-level privileges are not honored when JOINs are involved (you
must have the necessary table-level privileges, as you do
Hello Stephen,
Stephen Frost wrote:
I'm going to look into it but it's a bit complicated. I was hoping
someone who's more familiar with those parts would be able to look at
it.
Good to hear. I've just been asking, because it remained unclear to me.
I don't think that's the right approach,
Markus Wanner mar...@bluegap.ch writes:
Stephen Frost wrote:
BTW: how are long constant strings expected to be formatted? Are those
allowed to exceed 80 columns, or are they expected to be split like so
Honestly, I think I've seen both done.
Yeah, that's why I'm asking.
IMHO, the trouble
Hello Stephen,
Stephen Frost wrote:
..in the attached patch.
Thanks, I've reviewed this patch again.
Please find attached an updated patch for column-level privileges
which incorporates Alvaro's suggested changes and is updated to the
latest CVS HEAD.
Cool, applies cleanly and compiles
On Fri, Jan 2, 2009 at 16:11, Stephen Frost sfr...@snowman.net wrote:
* Stephen Frost (sfr...@snowman.net) wrote:
Please find attached an updated patch for column-level privileges
which incorporates Alvaro's suggested changes and is updated to the
latest CVS HEAD.
Hi!
This gives me
Alex,
* Alex Hunsaker (bada...@gmail.com) wrote:
Now it looks bogos but why not just move errmsg_col down to where we
actually use it? Or am I missing something?
Honestly, I think at this point is makes the most sense to just remove
it entirely, which I've done in the attached patch.
* Stephen Frost (sfr...@snowman.net) wrote:
Please find attached an updated patch for column-level privileges
which incorporates Alvaro's suggested changes and is updated to the
latest CVS HEAD. Regression tests have been added as well as
documentation (though this could probably be
50 matches
Mail list logo
