Re: How about to have relnamespace and relrole? - Mailing list pgsql-hackers

Hello, thank you for reviewing.

The attatched are the third version of this patch.

0001-Add-regrole_v3.patch
0002-Add-regnamespace_v3.patch

- Rearranged into regrole patch and regnamespace patch as seen above, each of them consists of changes for code, docs,
regtests.regnamespace patch depends on the another patch.
 

- Removed the irrelevant change and corrected mistakes in comments.

- Renamed role_or_oid to role_name_or_oid in regrolein().

- Changed the example name for regrole in the docmentation to 'smithee' as an impossible-in-reality-but-well-known
name,from 'john', the most common in US (according to Wikipedia).
 



At Tue, 24 Feb 2015 16:30:44 +0530, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote in
<CAM2+6=V-fwQFkwF0Pi3Dacq-ZRY5WxcPLVLuqFKf2Mf57_6inA@mail.gmail.com>
> I agree on Tom's concern on MVCC issue, but we already hit that when we
> introduced regclass and others. So I see no additional issue with these
> changes as such. About planner slowness, I guess updated documentation looks
> perfect for that.
> 
> So I went ahead reviewing these patches.
> 
> All patches are straight forward and since we have similar code already
> exists, I did not find any issue at code level. They are consistent with
> other
> functions.

One concern is about arbitrary allocation of OIDs for the new
objects - types, functions. They are isolated from other similar
reg* types, but there's no help for it without global renumbering
of static(system) OIDs.

> Patches applies with patch -p1. make, make install, make check has
> no issues. Testing was fine too.
> 
> However here are few review comments on the patches attached:
> 
> Review points on 0001-Add-regrole.patch
> ---
> 1.
> +#include "utils/acl.h"
> 
> Can you please add it at appropriate place as #include list is an ordered
> list

regrolein calls reg_role_oid in acl.c, which is declared in acl.h.

> 2.
> +    char       *role_or_oid = PG_GETARG_CSTRING(0);
> 
> Can we have variable named as role_name_or_oid, like other similar
> functions?

I might somehow have thought it a bit long. Fixed.

> 3.
> +    /*
> +     * Normal case: parse the name into components and see if it matches
> any
> +     * pg_role entries in the current search path.
> +     */
> 
> I believe, roles are never searched per search path. Thus need to update
> comments here.

Ouch. I forgot to edit it properly. I edit it. The correct
catalog name is pg_authid.

+    /* Normal case: see if the name matches any pg_authid entry. */

I also edited comments for regnamespacein.


> Review points on 0002-Add-regnamespace.patch
> ---
> 1.
> + * regnamespacein        - converts "classname" to class OID
> 
> Typos. Should be nspname instead of classname and namespase OID instead of
> class OID

Thank you for pointing out. I fixed the same mistake in
regrolein, p_ts_dict was there instead of pg_authid..  The names
of oid kinds appear in multiple forms, that is, oprname and
opr_name. Although I don't understand the reason, I followed the
convention.

> Review points on 0003-Check-new....patch
> ---
> 1.
> @@ -1860,6 +1860,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
>      Oid            attrdefOid;
>      ObjectAddress colobject,
>                  defobject;
> +    Oid            exprtype;
> 
> This seems unrelated. Please remove.

It's a trace of the previous code to ruling out the new oid
types. Removed.

> Apart from this, it will be good if you have ONLY two patches,
> (1) For regrole and (2) For regnamespace specific
> With all related changes into one patch. I mean, all role related changes
> i.e.
> 0001 + 0003 role related changes + 0004 role related changes and docs update
> AND
> 0002 + 0003 nsp related changes + 0004 nsp related changes

I prudently separated it since I wasn't confident on the
pertinence of ruling out. I rearranged them as your advise
including docs.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center