|From:||Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>|
|Cc:||Jim(dot)Nasby(at)bluetreble(dot)com, tgl(at)sss(dot)pgh(dot)pa(dot)us, pgsql-hackers(at)postgresql(dot)org|
|Subject:||Re: How about to have relnamespace and relrole?|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Hello, thank you for reviewing.
The attatched are the third version of this 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
- 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
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
> +#include "utils/acl.h"
> Can you please add it at appropriate place as #include list is an ordered
regrolein calls reg_role_oid in acl.c, which is declared in acl.h.
> + char *role_or_oid = PG_GETARG_CSTRING(0);
> Can we have variable named as role_name_or_oid, like other similar
I might somehow have thought it a bit long. Fixed.
> + /*
> + * Normal case: parse the name into components and see if it matches
> + * 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
> + * 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
> Review points on 0003-Check-new....patch
> @@ -1860,6 +1860,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
> Oid attrdefOid;
> ObjectAddress colobject,
> + Oid exprtype;
> This seems unrelated. Please remove.
It's a trace of the previous code to ruling out the new oid
> 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
> 0001 + 0003 role related changes + 0004 role related changes and docs update
> 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
NTT Open Source Software Center
|Next Message||Kyotaro HORIGUCHI||2015-02-26 10:34:37||Re: How about to have relnamespace and relrole?|
|Previous Message||Fujii Masao||2015-02-26 10:11:30||Re: Refactoring GUC unit conversions|