On Sat, May 20, 2017 at 8:40 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, May 19, 2017 at 3:01 PM, Masahiko Sawada <sawada.m...@gmail.com> > wrote: >> Also, as Horiguchi-san pointed out earlier, walreceiver seems need the >> similar fix. > > Actually, now that I look at it, ready_to_display should as well be > protected by the lock of the WAL receiver, so it is incorrectly placed > in walreceiver.h. As you are pointing out, pg_stat_get_wal_receiver() > is lazy as well, and that's new in 10, so we have an open item here > for both of them. And I am the author for both things. No issues > spotted in walreceiverfuncs.c after review. > > I am adding an open item so as both issues are fixed in PG10. With the > WAL sender part, I think that this should be a group shot. > > So what do you think about the attached?
I think if you're going to fix it so that we take spinlocks on MyWalSnd in a bunch of places that we didn't take them before, it would make sense to fix all the places where we're accessing those fields without a spinlock instead of only some of them, unless there are good reasons why we only need it in some cases and not others, in which case I think the patch should add comments to each place where the lock was not taken explaining why it's OK. It didn't take me and my copy of vi very long to find instances that you didn't change. I also think that should provide some analysis regarding the question Thomas asked - are these actually bugs, or are they OK for some reason? We shouldn't just plaster the code with spinlock acquisitions unless it's really necessary. It seems at least possible that these changes could cause performance regressions, and it doesn't look like you've done any testing or provided any analysis indicating whether that's likely to happen or not. Basically, I don't think this patch is ready to commit. We have neither evidence that it is necessary nor evidence that it is complete. I don't want to be unfair, here, but it seems to me you ought to do a little more legwork before throwing this over the wall to the pool of committers. [ Side note: Erik's report on this thread initially seemed to suggest that we needed this patch to make logical decoding stable. But my impression is that this is belied by subsequent developments on other threads, so my theory is that this patch was never really related to the problem, but rather than by the time Erik got around to testing this patch, other fixes had made the problems relatively rare, and the apparently-improved results with this patch were just chance. If that theory is wrong, it would be good to hear about it. ] -- 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