Re: refactoring comment.c

(2010/08/07 0:02), Robert Haas wrote:
> At PGCon, we discussed the possibility that a minimal SE-PostgreSQL
> implementation would need little more than a hook in
> ExecCheckRTPerms() [which we've since added] and a security label
> facility [for which KaiGai has submitted a patch].  I actually sat
> down to write the security label patch myself while we were in Ottawa,
> but quickly ran into difficulties: while the hook we have now can't do
> anything useful with objects other than relations, it's pretty clear
> from previous discussions on this topic that the demand for labels on
> other kinds of objects is not going to go away.  Rather than adding
> additional syntax to every object type in the system (some of which
> don't even have ALTER commands at present), I suggested basing the
> syntax on the existing COMMENT syntax.  After some discussion[1], we
> seem to have settled on the following:
>
> SECURITY LABEL [ FOR<provider>  ] ON<object class>  <object name>  IS '<label>';
>
> At present, there are some difficulties with generalizing this syntax
> to other object types.  As I found out when I initially set out to
> write this patch, it'd basically require duplicating all of comment.c,
> which is an unpleasant prospect, because that file is big and crufty;
> it has a large amount of internal duplication.  Furthermore, the
> existing locking mechanism that we're using for comments is known to
> be inadequate[2].  Dropping a comment while someone else is in the
> midst of commenting on it leaves orphaned comments lying around in
> pg_(sh)description that could later be inherited by a new object.
> That's a minor nuisance for comments and would be nice to fix, but is
> obviously a far larger problem for security labels, where even a small
> chance of randomly mislabeling an object is no good.
>
> So I wrote a patch.  The attached patch factors out all of the code in
> comment.c that is responsible for translating parser representations
> into a new file parser/parse_object.c, leaving just the
> comment-specific stuff in commands/comment.c.  It also adds
> appropriate locking, so that concurrent COMMENT/DROP scenarios don't
> leave behind leftovers.  It's a fairly large patch, but the changes
> are extremely localized: comment.c gets a lot smaller, and
> parse_object.c gets bigger by a slightly smaller amount.
>
> Any comments?  (ha ha ha...)
>
> [1] http://archives.postgresql.org/pgsql-hackers/2010-07/msg01328.php
> [2] http://archives.postgresql.org/pgsql-hackers/2010-07/msg00351.php
>

Thanks for your efforts.
I believe the get_object_address() enables to implement security
label features on various kind of database objects.

I tried to look at the patch. Most part is fine, but I found out
two issues.

On the object_exists(), when we verify existence of a large object,
it needs to scan pg_largeobject_metadata, instead of pg_largeobject.
When we implement pg_largeobject_metadata catalog, we decided to set
LargeObjectRelationId on object.classId due to the backend compatibility.

|    /*
|     * For object types that have a relevant syscache, we use it; for
|     * everything else, we'll have to do an index-scan.  This switch
|     * sets either the cache to be used for the syscache lookup, or the
|     * index to be used for the index scan.
|     */
|    switch (address.classId)
|    {
|        case RelationRelationId:
|            cache = RELOID;
|            break;
|              :
|        case LargeObjectRelationId:
|            indexoid = LargeObjectMetadataOidIndexId;
|            break;
|              :
|     }
|
|    /* Found a syscache? */
|    if (cache != -1)
|        return SearchSysCacheExists1(cache, ObjectIdGetDatum(address.objectId));
|
|    /* No syscache, so examine the table directly. */
|    Assert(OidIsValid(indexoid));
|    ScanKeyInit(&skey[0], ObjectIdAttributeNumber, BTEqualStrategyNumber,
|                F_OIDEQ, ObjectIdGetDatum(address.objectId));
|    rel = heap_open(address.classId, AccessShareLock);                     ^^^^^^^^^^^^^^^ <- It tries to open
pg_largeobject
|    sd = systable_beginscan(rel, indexoid, true, SnapshotNow, 1, skey);
|    found = HeapTupleIsValid(systable_getnext(sd));
|    systable_endscan(sd);
|    heap_close(rel, AccessShareLock);
|    return found;
| }


Although no caller invokes get_object_address() with lockmode = NoLock,
isn't it necessary to skip locking if NoLock was given.

|    /*
|     * If we're dealing with a relation or attribute, then the relation is
|     * already locked.  If we're dealing with any other type of object, we need
|     * to lock it and then verify that it still exists.
|     */
|    if (address.classId != RelationRelationId)
|    {
|        if (IsSharedRelation(address.classId))
|            LockSharedObject(address.classId, address.objectId, 0, lockmode);
|        else
|            LockDatabaseObject(address.classId, address.objectId, 0, lockmode);
|        /* Did it go away while we were waiting for the lock? */
|        if (!object_exists(address))
|            elog(ERROR, "cache lookup failed for class %u object %u subobj %d",
|                 address.classId, address.objectId, address.objectSubId);
|    }

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>