Re: Drastic performance loss in assert-enabled build in HEAD

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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