Hello,
At Thu, 30 Apr 2015 17:12:25 -0300, Alvaro Herrera
wrote in <20150430201225.gv4...@alvh.no-ip.org>
> Kyotaro HORIGUCHI wrote:
> > Thank you for completing this and very sorry not to respond these
> > days.
> >
> > I understood that it is committed after I noticed that rebasing
> > my cod
Kyotaro HORIGUCHI wrote:
> Thank you for completing this and very sorry not to respond these
> days.
>
> I understood that it is committed after I noticed that rebasing
> my code failed..
You'd do well to check your email, I guess :-)
> Although after committed, I found some issues as I looked o
Thank you for completing this and very sorry not to respond these
days.
I understood that it is committed after I noticed that rebasing
my code failed..
Although after committed, I found some issues as I looked on
it. Please forgive me to comment it now after all this time.
Several changes in do
Alvaro Herrera wrote:
> With this patch applied, doing
> \h ALTER ROLE
> in psql looks quite odd: note how wide it has become. Maybe we should
> be doing this differently? (Hmm, why don't we accept ALL in the first SET
> line? Maybe that's just a mistake and the four lines should be all
> ident
There is something odd here going on:
alvherre=# alter group test add user current_user;
ERROR: role "test" is a member of role "(null)"
Surely (null) is not the right name to be reporting there ...
Attached is a rebased patch, which also has some incomplete doc changes.
With this patch applie
Actually this is better -- I added token location tracking, and changed
RoleId to use RoleSpec which means it can throw errors with locations
when "public" or "none" are specified. I think the checks for
public/none in CreateRole and AlterRole are dead code now.
--
Álvaro Herrera
Alvaro Herrera wrote:
> Kyotaro HORIGUCHI wrote:
> Thanks for doing the fiddly work here. Attached is a new version of
> this patch. I simplified some things, including removing those rules
> you added to RoleId. It seems to me that this problem:
>
> > RoleId in the patch still has rule compon
Kyotaro HORIGUCHI wrote:
> Hello, thank you for the comment.
>
> > Looking at this a bit, I'm not sure completely replacing RoleId with a
> > node is the best idea; some of the users of that production in the
> > grammar are okay with accepting only normal strings as names, and don't
> > need all
Hello, thank you for the comment.
> Looking at this a bit, I'm not sure completely replacing RoleId with a
> node is the best idea; some of the users of that production in the
> grammar are okay with accepting only normal strings as names, and don't
> need all the CURRENT_USER etc stuff.
You're r
Looking at this a bit, I'm not sure completely replacing RoleId with a
node is the best idea; some of the users of that production in the
grammar are okay with accepting only normal strings as names, and don't
need all the CURRENT_USER etc stuff. Maybe we need a new production,
say RoleSpec, and w
Thank you.
> A new patch has been sent here but no review occurred, hence moving
> this item to CF 2014-12.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.po
On Fri, Nov 14, 2014 at 5:39 PM, Kyotaro HORIGUCHI
wrote:
> Hi, this is revised version.
>
>> Kyotaro HORIGUCHI wrote:
>>
>> > - Storage for new information
>> >
>> > The new struct NameId stores an identifier which telling what it
>> > logically is using the new enum NameIdTypes.
>>
>> I think Na
Hi, this is revised version.
> Kyotaro HORIGUCHI wrote:
>
> > - Storage for new information
> >
> > The new struct NameId stores an identifier which telling what it
> > logically is using the new enum NameIdTypes.
>
> I think NameId is a bad name for this. My point is that NameId, as it
> stan
Kyotaro HORIGUCHI wrote:
> - Storage for new information
>
> The new struct NameId stores an identifier which telling what it
> logically is using the new enum NameIdTypes.
I think NameId is a bad name for this. My point is that NameId, as it
stands, might be a name for anything, not just a rol
Hello, This is the new version. (WIP v2)
The first attachment is the patch and the second is test sql
script.
- Behavior changing
Almost all syntax taking role accepts CURRENT_USER and
SESSION_USER and they are distinguished from "current_user" and
"session_user". The exceptions are follows.
-
Hello,
> Adam Brightwell writes:
> > FWIW, I found the following in the archives:
>
> > http://www.postgresql.org/message-id/15516.1038718...@sss.pgh.pa.us
>
> > Now this is from 2002 and it appears it wasn't necessary to change at the
> > time, but I haven't yet found anything else related (it
All,
> I agree that we should probably seperate the concerns here. Personally,
> I like the idea of being able to say "CURRENT_USER" in utility commands
> to refer to the current user where a role would normally be expected, as
> I could see it simplifying things for some applications, but that'
Adam Brightwell writes:
> FWIW, I found the following in the archives:
> http://www.postgresql.org/message-id/15516.1038718...@sss.pgh.pa.us
> Now this is from 2002 and it appears it wasn't necessary to change at the
> time, but I haven't yet found anything else related (it's a big archive).
> T
Stephen Frost writes:
> The other idea which comes to mind is- could we try to actually resolve
> what the 'right' answer is here, instead of setting a special value and
> then having to detect and fix it later?
No, absolutely not. Read the NOTES at the head of gram.y. Or if you
need it spelled
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote:
> > | RoleId_or_curruser: RoleId{ $$ = $1; }
> > | | CURRENT_USER { $$ = "\x00\x01";};
[...]
> > This is ugly but needs no additional struct member or special
> > logics. (Macros c
Kyotaro,
Zero-length identifiers are rejected in scanner so RoleId cannot
> be a valid pointer to a zero-length string. (NULL is used as
> PUBLIC in auth_ident)
>
> | postgres=# create role "";
> | ERROR: zero-length delimited identifier at or near
> | postgres=# create role U&"\00";
> | ERR
Hello,
At Tue, 28 Oct 2014 09:05:20 -0400, Stephen Frost wrote in
<20141028130520.gl28...@tamriel.snowman.net>
> > As well, the
> > originally proposed "RoleId_or_curruser" suffers from the same issue. I'm
> > going to go out on a limb here, but is it not possible to consider
> > "current_user"
Hello, thank you all for many comments.
At the first, I removed changes for role-vs-user consistency and
remove all added role named other than current_user.
The followings are one-by-one answer for the comments so far,
please let me know if I missed anything.
- The necessity of the new function
Marti Raudsepp wrote:
> On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
> wrote:
> > - 0001-ALTER-ROLE-CURRENT_USER_v2.patch - the patch.
>
> +RoleId:CURRENT_USER{ $$ =
> "current_user";}
> + | USER { $$ = "curr
* Kevin Grittner (kgri...@ymail.com) wrote:
> Stephen Frost wrote:
>
> > You can still double-quote reserved words and use them in general. What
> > we're talking about here are cases where a word can't be used even if
> > it's double-quoted, and we try really hard to keep those cases at an
> >
On Tue, Oct 28, 2014 at 2:40 AM, Adam Brightwell
wrote:
> Taking a step back, I'm still not sure I understand the need for this
> feature or the use case. It seems to have started as a potential fix to an
> inconsistency between ALTER USER and ALTER ROLE syntax (which I think I
> could see some v
Stephen Frost wrote:
> You can still double-quote reserved words and use them in general. What
> we're talking about here are cases where a word can't be used even if
> it's double-quoted, and we try really hard to keep those cases at an
> absolute minimum. We should also really be keeping a li
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote:
> > +RoleId:CURRENT_USER{ $$ =
> > "current_user";}
> > + | USER { $$ = "current_user";}
> > + | CURRENT_ROLE { $$ =
>
> +RoleId:CURRENT_USER{ $$ =
> "current_user";}
> + | USER { $$ = "current_user";}
> + | CURRENT_ROLE { $$ = "current_user";}
> + | SESSION_USER { $
Marti Raudsepp wrote
> On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
> <
> horiguchi.kyotaro@.co
> > wrote:
>
> But should ALTER USER ALL and ALTER ROLE ALL really do the same thing?
> A user is a role with the LOGIN option. Every user is a role, but not
> every role is a user. I suspect t
On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
wrote:
> - 0001-ALTER-ROLE-CURRENT_USER_v2.patch - the patch.
+RoleId:CURRENT_USER{ $$ = "current_user";}
+ | USER { $$ = "current_user";}
+ | CURRENT_ROL
All,
I just ran through the patch giving it a good once over, some items to
address/consider/discuss:
* Trailing whitespace.
* Why are you making changes in foreigncmds.c? These seem like unnecessary
changes. I see that you are trying to consolidate but this refactor seems
potentially out of sc
Thanks Kyotaro,
I just did quickly looked at the patch and it does cover more syntax then
earlier patch. But still if doesn't not cover the all the part of syntax
where
we can use CURRENT_USER/CURRENT_ROLE/USER/SESSION_USER. For example:
-- Not working
alter default privileges for role current_us
Hi, here is the revised patch.
Attached files are the followings
- 0001-ALTER-ROLE-CURRENT_USER_v2.patch - the patch.
- testset.tar.bz2 - test set. Run by typing 'make check' as a
superuser of the running postgreSQL server. It creates "testdb"
and some roles.
Documents are not edited t
Hello,
> Kyotaro,
>
> Food for thought. Couldn't you reduce the following block:
>
> + if (strcmp(stmt->role, "current_user") == 0)
> + {
> + roleid = GetUserId();
> + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
> + if (!HeapTupleIsValid(tuple))
> + ereport(ERROR,
> + (errcode(E
Thank you for reviewing,
2014 13:10:57 +0530, Rushabh Lathia wrote in
> I gone through patch and here is the review for this patch:
>
>
> .) patch go applied on master branch with patch -p1 command
>(git apply failed)
> .) regression make check run fine
> .) testcase coverage is missing
Kyotaro,
Food for thought. Couldn't you reduce the following block:
+ if (strcmp(stmt->role, "current_user") == 0)
+ {
+ roleid = GetUserId();
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ er
I gone through patch and here is the review for this patch:
.) patch go applied on master branch with patch -p1 command
(git apply failed)
.) regression make check run fine
.) testcase coverage is missing in the patch
.) Over all coding/patch looks good.
Few comments:
1) Any particular reaso
38 matches
Mail list logo
