Thread: [HACKERS] Leftover invalidation handling in RemoveRelations
Hi,
reviewing some citus code copied from postgres I noticed that
RemoveRelations() has the following bit:
/* * These next few steps are a great deal like relation_openrv, but we * don't bother building a relcache
entrysince we don't need it. * * Check for shared-cache-inval messages before trying to access the *
relation. This is needed to cover the case where the name * identifies a rel that has been dropped and recreated
sincethe * start of our transaction: if we don't flush the old syscache entry, * then we'll latch onto that
entryand suffer an error later. */ AcceptInvalidationMessages();
/* Look up the appropriate relation using namespace search. */ state.relkind = relkind; state.heapOid =
InvalidOid; state.concurrent = drop->concurrent; relOid = RangeVarGetRelidExtended(rel, lockmode, true,
false, RangeVarCallbackForDropRelation,
(void *) &state);
which doesn't seem to make sense - RangeVarGetRelidExtended does
invalidation handling on it's own.
Looks like this was left there in the course of
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ad36c4e44c8b513f6155656e1b7a8d26715bb94
ISTM AcceptInvalidationMessages() and preceding comment should just be
removed?
Greetings,
Andres Freund
On Wed, Mar 15, 2017 at 2:40 PM, Andres Freund <andres@anarazel.de> wrote: > reviewing some citus code copied from postgres I noticed that > RemoveRelations() has the following bit: > > /* > * These next few steps are a great deal like relation_openrv, but we > * don't bother building a relcache entry since we don't need it. > * > * Check for shared-cache-inval messages before trying to access the > * relation. This is needed to cover the case where the name > * identifies a rel that has been dropped and recreated since the > * start of our transaction: if we don't flush the old syscache entry, > * then we'll latch onto that entry and suffer an error later. > */ > AcceptInvalidationMessages(); > > /* Look up the appropriate relation using namespace search. */ > state.relkind = relkind; > state.heapOid = InvalidOid; > state.concurrent = drop->concurrent; > relOid = RangeVarGetRelidExtended(rel, lockmode, true, > false, > RangeVarCallbackForDropRelation, > (void *) &state); > > which doesn't seem to make sense - RangeVarGetRelidExtended does > invalidation handling on it's own. > > Looks like this was left there in the course of > http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ad36c4e44c8b513f6155656e1b7a8d26715bb94 > > ISTM AcceptInvalidationMessages() and preceding comment should just be > removed? Yeah, I don't think that would hurt anything. (I'm not sure it'll help anything either - the overhead of an extra AcceptInvalidationMessages() call is quite minimal - but, as you say, maybe it's worth doing just to discourage future code authors from including unnecessary fluff.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 2017-03-15 14:46:22 -0400, Robert Haas wrote: > On Wed, Mar 15, 2017 at 2:40 PM, Andres Freund <andres@anarazel.de> wrote: > Yeah, I don't think that would hurt anything. > > (I'm not sure it'll help anything either - the overhead of an extra > AcceptInvalidationMessages() call is quite minimal - but, as you say, > maybe it's worth doing just to discourage future code authors from > including unnecessary fluff.) I don't think there's an actual runtime advantage either - but it's indeed confusing for others, because it doesn't square with what's needed. It's not like the AcceptInvalidationMessages() would actually make things race-free if used without RangeVarGetRelidExtended()... - Andres