Robert Haas wrote:
> On Sat, Oct 17, 2015 at 5:37 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> > I agree with that sentiment.
> >
> > Attached patch adds variable to the shmem which is used for module
> > activation tracking - set to true in ActiveCommitTs() and false in
> > DeactivateCommitTs(). All the checks inside the commit_ts code were changed
> > to use this new variable. I also removed the static variable Alvaro added in
> > previous commit because it's not needed anymore.
>
> That sounds good to me. On a quick read-through it looks OK too.
A revised version is attached. Two changes on top of Petr's patch:
1. In the two "get" routines, we were reading the flag without grabbing
the lock. This is okay in a master server, because the flag cannot
change in flight, but in a standby it is possible to have the module
be deactivated while TS data is being queried. To fix this, simply move
the check for the active shmem flag a few lines down to be inside the
locked section.
There are two other places that also read the flag without grabbing the
lock. These look okay to me, so I added comments stating so.
2. In TransactionIdGetCommitTsData() we were grabbing lock, reading some
data, releasing lock, then examining the "cached" value in shmem without
a lock to see if it matched the function argument; if it's match, grab
lock again and return the correct data. In the original coding this
made sense because there was no locked section prior to reading the
cache, but after the patch this was pointless. Make it simpler by
moving the read of the cache inside the locked section too.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services