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 bett
On Thu, Jan 15, 2009 at 12:33 AM, KaiGai Kohei 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 preference. Pl
>> 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
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 c
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 database
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 an
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 whet
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
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 setTargetTa
Stephen Frost 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. If a colum
* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost 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.
Stephen Frost 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 behavior
is correc
* 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 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 being tested i
Stephen Frost 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 wouldn't it be a
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
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() */
> rte-
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 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;
Indee
KaiGai Kohei 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 place to do this.
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(
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;
GRA
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
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 a
Stephen Frost 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 patch used, ie of
Jamie, et al,
* Jaime Casanova (jcasa...@systemguards.com.ec) wrote:
>
> ! Currently, PostgreSQL does not recognize
> ! column-level SELECT privileges when a JOIN is involved.
> One possible workaround is to create a view having just the desired
> columns and then grant p
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
ma
On Thu, Jan 8, 2009 at 11:34 PM, Stephen Frost 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
! Currently, PostgreSQL does not recognize
! column-level SELECT priv
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
>
Stephen Frost wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Stephen Frost 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
* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost 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 e
Stephen Frost 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 ExecCheckRTEPerms() to han
Tom, et al,
* Tom Lane (t...@sss.pgh.pa.us) wrote:
> KaiGai Kohei 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
KaiGai Kohei 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 applies result of
Jaime Casanova wrote:
On Wed, Jan 7, 2009 at 1:46 AM, KaiGai Kohei 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 list
On Wed, Jan 7, 2009 at 1:46 AM, KaiGai Kohei 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 list.
>
for my t
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 alo
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
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
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
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.
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 th
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 cl
Markus Wanner 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 with b
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 approac
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,
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
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.
On Fri, Jan 2, 2009 at 16:11, Stephen Frost 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
aclchk.c: In fu
* 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 b
50 matches
Mail list logo
