Kevin Grittner <kgri...@ymail.com> wrote: > Tom Lane <t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
