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(at)kaigai(dot)gr(dot)jp>