On Fri, Nov 14, 2014 at 5:39 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> 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 NameId is a bad name for this. My point is that NameId, as it
>> stands, might be a name for anything, not just a role; and the object it
>> identifies is not an Id either. Maybe RoleSpec?
>
> Yeah! I felt it no good even if it were a generic type for
> various "Name of something or its oid". RoleSpec sounds much better.
>
>> Do we need a public_ok
>> argument to get_nameid_oid() (get a better name for this function too)
>
> Maybe get_rolespec_oid() as a name ofter its parameter type?
>
>> so that callers don't have to check for InvalidOid argument? I think
>> the arrangement you propose is not very convenient; it'd be best to
>> avoid duplicating the check for InvalidOid in all callers of the new
>> function, particularly where there was no check before.
>
> I agree that It'd be better keeping away from duplicated
> InvalidOid checks, but public_ok seems a bit myopic. Since
> there's no reasonable border between functions accepting 'public'
> and others, such kind of solution would not be reasonable..
>
> What about checking it being a PUBLIC or not *before* calling
> get_rolespec_oid()?
>
> The attached patch modified in the following points.
>
> - rename NameId to RoleSpec and NameIdType to RoleSpecTypes.
> - rename get_nameid_oid() to get_rolespec_oid().
> - rename roleNamesToIds() to roleSpecsToIds().
> - some struct members are changed such as authname to authrole.
> - check if rolespec is "public" or not before calling get_rolespec_oid()
> - ExecAlterDefaultPrivilegesStmt and ExecuteGrantStmt does
> slightly different things about ACL_ID_PUBLIC but I unified it
> to the latter.
> - rebased to the current master
A new patch has been sent here but no review occurred, hence moving
this item to CF 2014-12.
--
Michael