Re: How about to have relnamespace and relrole?

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(dot)chalke(at)enterprisedb(dot)com> wrote in <CAM2+6=V-fwQFkwF0Pi3Dacq-ZRY5WxcPLVLuqFKf2Mf57_6inA(at)mail(dot)gmail(dot)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