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 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?
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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=2ad36c4e44c8b513f6155656e1b7a8d26715bb94ISTM 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers