Re: [HACKERS] Race conditions with WAL sender PID lookups - Mailing list pgsql-hackers

From Alvaro Herrera
Subject Re: [HACKERS] Race conditions with WAL sender PID lookups
Date
Msg-id 20170703161455.7ele6byeeiw2mvmw@alvherre.pgsql
Whole thread Raw
In response to Re: [HACKERS] Race conditions with WAL sender PID lookups  (Michael Paquier <michael.paquier@gmail.com>)
Responses Re: [HACKERS] Race conditions with WAL sender PID lookups
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Chapman Flack
Date:
Subject: Re: [HACKERS] AdvanceXLInsertBuffer vs. WAL segment compressibility
Next
From: Emrul
Date:
Subject: [HACKERS] Revisiting NAMEDATALEN