Re: Fwd: Proposal: variant of regclass - Mailing list pgsql-hackers

Hi Amit Kapila,

Thank you for your reviewing. I updated the patch to v5.
This differences from the previous version are:

- Revise documents and comments acording with your reviews
- Fix not to avoid an error in case of "type xxxx is only a shell"
- Fix regclass_guts and regtype_guts to close the scan and relation when the
  object is not found in bootstrap mode
- Fix OpernameGetCandidates to check missing_schema_ok before returning NULL
- Move _guts functions into /* SUPPORT ROUTINES */ section
- Fix oids for recent master.

With Regards,
Yugo Nagata

On Sun, 30 Mar 2014 09:26:42 +0530
Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> >> Thanks for your a lot of comments. I revised the patch according to
> >> comments from Robert Haas and Marti Raudsepp.
> >
> > I have started looking into this patch and below are my
> > initial findings:
>
> I have looked further into this patch and below are some more findings.
> This completes my review for this patch.
>
> 1.
> regclass_guts(..)
> {
> ..
> if (HeapTupleIsValid(tuple = systable_getnext(sysscan)))
> *classid_p = HeapTupleGetOid(tuple);
> else
> return false;
>
> /* We assume there can be only one match */
>
> systable_endscan(sysscan);
> heap_close(hdesc, AccessShareLock);
>
> }
>
> In case tuple is not found, false is returned without closing the scan and
> relation, now if error is returned it might be okay, because it will release
> lock during abort, but if error is not returned  (in case of to_regclass),
> it will be considered as leak. I think it might not effect anything directly,
> because we are not using to_regclass() in Bootstrap mode, but still I feel
> it is better to avoid such a leak in API. We can handle it similar to how it
> is done in regproc_guts(). The similar improvement is required in
> regtype_guts().
>
> 2.
> ! OpernameGetCandidates(List *names, char oprkind, bool missing_schema_ok)
> {
> ..
> ! namespaceId = LookupExplicitNamespace(schemaname, missing_schema_ok);
> ! if (!OidIsValid(namespaceId))
> ! return NULL;
>
> }
>
> In this check it is better to check missing_schema_ok along with
> invalid oid check, before returning NULL.
>
> 3.
> /*
>  * to_regproc - converts "proname" to proc OID
>  *
>  * Diffrence from regprocin is, this does not throw an error and returns NULL
>  * if the proname does not exist.
>  * Note: this is not an I/O function.
>  */
>
> I think function header comments can be improved for all (to_*) newly
> added functions. You can refer function header comments for
> simple_heap_insert
> which explains it's difference from heap_insert.
>
>
> 4. Oid's used for newly added functions became duplicate after a recent checkin.
>
> 5. This file is divided into /* SUPPORT ROUTINES*/ and USER I/O ROUTINES
>    isn't it better to move _guts functions into Support Routines.
>
> 6.
> ! parseTypeString(const char *str, Oid *typeid_p, int32 *typmod_p,
> bool missing_ok)
>
> Line length greater than 80
>
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com


--
Yugo Nagata <nagata@sraoss.co.jp>