Sorry, I fixed a silly typo in documentation in the previous version. - of theses types has a significance... + of these types has a significance...
# My fingers frequently slip as above.. I incremented the version of this revised patch to get rid of confusion. ======= Hello, thank you for reviewing. The attatched are the fourth version of this patch. 0001-Add-regrole_v4.patch 0002-Add-regnamespace_v4.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.cha...@enterprisedb.com> wrote in <CAM2+6=v-fwqfkwf0pi3dacq-zry5wxcplvluqfkf2mf57_6...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
