Kevin Grittner <kgrittn@ymail.com> wrote:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Another reason why I don't like this code is that
>> pg_relation_is_scannable is broken by design:
>>
>> relid = PG_GETARG_OID(0);
>> relation = RelationIdGetRelation(relid);
>> result = relation->rd_isscannable;
>> RelationClose(relation);
>>
>> You can't do that: if the relcache entry doesn't already exist,
>> this will try to construct one while not holding any lock on the
>> relation, which is subject to all sorts of race conditions.
>
> Hmm. I think I had that covered earlier but messed up in
> rearranging to respond to review comments. Will review both new
> calling locations.
For the SQL-level function, does this look OK?:
diff --git a/src/backend/utils/adt/dbsize.c
b/src/backend/utils/adt/dbsize.c
index d589d26..94e55f0 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -850,9 +850,13 @@ pg_relation_is_scannable(PG_FUNCTION_ARGS)
bool result;
relid = PG_GETARG_OID(0);
- relation = RelationIdGetRelation(relid);
+ relation = try_relation_open(relid, AccessShareLock);
+
+ if (relation == NULL)
+ PG_RETURN_BOOL(false);
+
result = relation->rd_isscannable;
- RelationClose(relation);
+ relation_close(relation, AccessShareLock);
PG_RETURN_BOOL(result);
}
I think the call in ExecCheckRelationsScannable() is safe because
it comes after the tables are all already locked. I put it there
so that the appropriate lock strength should be used based on the
whether it was locked by ExecInitNode() or something before it. Am
I missing something? Can I not count on the lock being held at
that point? Would the right level of API here be relation_open()
with NoLock rather than RelationIdGetRelation()? Or is there some
other call which is more appropriate there?
--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company