Re: Fwd: Proposal: variant of regclass

On Sat, Mar 22, 2014 at 1:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)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