Re: Make relation_openrv atomic wrt DDL

On Sun, Jun 12, 2011 at 3:18 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> Indeed, the original patch slowed it by about 50%.  I improved the patch,
> adding a global SharedInvalidMessageCounter to increment as we process
> messages.  If this counter does not change between the RangeVarGetRelid() call
> and the post-lock AcceptInvalidationMessages() call, we can skip the second
> RangeVarGetRelid() call.  With the updated patch, I get these timings (in ms)
> for runs of "SELECT nmtest(10000000)":
>
> master: 19697.642, 20087.477, 19748.995
> patched: 19723.716, 19879.538, 20257.671
>
> In other words, no significant difference.  Since the patch removes the
> no-longer-needed pre-lock call to AcceptInvalidationMessages(), changing to
> "relation_close(r, NoLock)" in the test case actually reveals a 15%
> performance improvement.  Granted, nothing to get excited about in light of
> the artificiality.

In point of fact, given the not-so-artificial results I just posted on
another thread ("lazy vxid locks") I'm *very* excited about trying to
reduce the cost of AcceptInvalidationMessages(). I haven't reviewed
your patch in detail, but is there a way we can encapsulate the
knowledge of the invalidation system down inside the sinval machinery,
rather than letting the heap code have to know directly about the
counter? Perhaps AIV() could return true or false depending on
whether any invalidation messages were processed, or somesuch.

> This semantic improvement would be hard to test with the current pg_regress
> suite, so I do not include any test case addition in the patch.  If
> sufficiently important, it could be done with isolationtester.

I haven't had a chance to look closely at the isolation tester yet,
but I'm excited about the possibilities for testing this sort of
thing. Not sure whether it's worth including this or not, but it
doesn't seem like a bad idea.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company