Subject: Re: Race conditions with WAL sender PID lookups

From: Michael Paquier <[email-masked]>
Date: Mon, 3 Jul 2017 14:05:46 +0900

On Sat, Jul 1, 2017 at 7:20 AM, Alvaro Herrera <[email-masked]> wrote:
> Michael Paquier wrote:
>> Considering how crazy the conditions to make the information fetched
>> by users inconsistent are met, I agree with that.
>
> Pushed.

Thanks Álvaro for pushing the patch. I had a second look and the
result looks good to me.

-       SpinLockAcquire(&walsnd->mutex);
+       }
+       pid = walsnd->pid;
The WAL receiver code used a cast to (int) in
pg_stat_get_wal_receiver(). I don't recall adding it. Why not being
consistent for both by removing the one of the WAL receiver code?

> In passing, clean up some leftover braces which were used to create
> unconditional blocks.  Once upon a time these were used for
> volatile-izing accesses to those shmem structs, which is no longer
> required.  Many other occurrences of this pattern remain.

Here are the places where a cleanup can happen:
- WalSndSetState
- ProcessStandbyReplyMessage
- XLogRead, 2 places
- XLogSendLogical
- WalRcvWaitForStartPosition
- WalRcvDie
- XLogWalRcvFlush
- ProcessWalSndrMessage
In most of the places of the WAL sender, braces could be removed to
improve the style. For the WAL receiver, declarations are not
necessary. As a matter of style, why not cleaning up just the WAL
sender stuff? Changing the WAL receiver code just to remove some
declarations would not improve readability, and would make back-patch
more difficult.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 3 Jul 2017 12:14:55 -0400

Michael Paquier wrote:

> pg_stat_get_wal_receiver(). I don't recall adding it.

I added it because it made me a bit nervous to pass a pid_t to
DatumGetInt32.  This one is assigning to a variable of type pid_t, so it
doesn't need a cast.

I'm not too clear on using pid_t variables as int32 Datum-packed
variables.  We don't do it a lot in the backend code (I found some
occurrences in contrib, but these don't inspire me a lot of confidence.)

> Why not being consistent for both by removing the one of the WAL
> receiver code?

I can't think of any reason to remove that cast.  It serves as
documentation if nothing else -- it alerts the reader that something is
going on.

> declarations would not improve readability, and would make back-patch

I think we should clean this up whenever we're modifying the surrounding
code, but otherwise we can leave well enough alone.  It's not a high
priority item at any rate.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

---

From: Noah Misch <[email-masked]>
Date: Mon, 3 Jul 2017 21:34:46 -0700

> priority item at any rate.

Bundling code cleanup into commits that also do something else is strictly
worse than bundling whitespace cleanup, which is itself bad:
https://postgr.es/m/flat/[email-masked]

Deferring cleanup and pushing cleanup-only commits are each good options.

---

From: Michael Paquier <[email-masked]>
Date: Fri, 7 Jul 2017 15:02:28 +0900

> https://postgr.es/m/flat/[email-masked]

FWIW, I agree with that. I favor as well separate commits for cleanups
and for fixes, so as each commit has its own goal and protects it.

(The cleanups discussed on this thread have been partially done in
commit 572d6ee where a bug has been fixed, not by me on the patches I
submitted ;) )
-- 
Michael