Michael Paquier wrote:
> 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.
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.
> > 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.
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
