Re: PATCH: pgbench - option to build using ppoll() for largerconnection counts

On 1/24/18, 00:00, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote:
    
    Hello Doug,

Hello Fabien,
   
    > This version of the patch attempts to address the feedback for the 
    > previous submission on 28-Nov-2017
    
    Please avoid recreating a thread, but rather respond to the previous one, 
    I missed this post.

Got it.
    
    The overall function-based implementation with limited ifdefs seems 
    readable and maintainable. I think that it rather improves the code in 
    places by hiding details.
    
    Patch applies with one warning:
    
       pgbench11-ppoll-v6.patch:141: trailing whitespace.
          set_socket(sockets, PQsocket(state[i].con), i);<SPACE>
       warning: 1 line adds whitespace errors.

Fixed.
    
    The patch implies to run "autoconf" to regenerate the configure script.

Yes. I should have included this in the description.
    
    Compilation with "ppoll" ok, globale & local make check ok. I do not have 
    hardware which allows to run with over 1024 clients, so I cannot say that 
    I tested the case.
    
    Compilation without "ppoll" gets a warning:
    
       pgbench.c:5645:1: warning: ‘clear_socket’ defined but not used...
         clear_socket(socket_array *sa, int fd, int idx)
    
    The "clear_socket" function is not used in this case. I'd suggest to 
    remove it from the prototypes, remove or comment the unused implementation 
    in the select section, and keep the one in the ppoll section. Or use it if 
    it is needed. Or inline it in the "init_socket_array" function where it is 
    used just once.

"clear_socket" removed. Functionality inlined into "init_socket_array"
    
    I'm not sure of the name "socket_array", because it is an array only in 
    the ppoll case. Maybe "sockets_t" or "socket_set"? Or something else?

Changed "socket_array" to "socket_set"
    
    Maybe "init_socket_array" should be named "clear_...".

Renamed "init_socket_array" to "clear_socket_set"
    
    I would suggest not to include sys/select.h when using ppoll, as it is a useless
    include this case. I.e. move includes in the ifdef USE_PPOLL section?

Done. Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.
    
    Please do not remove comments, eg:
    
       -  /* identify which client sockets should be checked for input */

Restored.
    
    On #endif or #else, especially large scope ones, I would have a comment to 
    say about what it is, eg at the minimum:
       #else /* select(2) implementation */
       #endif /* USE_PPOLL */
    
    On:
      +#if defined(USE_PPOLL)
      +#ifdef POLLRDHUP
    
    Use just one "ifdef" style?
    
    There should be a comment about what this sections are providing, eg:
       /* ppoll(2) implementation for "socket_array" functions */
    
    There should be an empty line and possibly a comment explaining why
    POLLRDHUP may not be there and/or why this define is necessary.

Removed unneeded #ifdef around POLLRDHUP
    
    With select you always initialize the timeout, but not with ppoll.
    Use a common approach in the implementations?

Fixed init & passing of timeout.
    
    The "finishCon" function addition seems totally unrelated to this patch. 
    Although I agree that this function improves the code, it is refactoring 
    and does not really belong here.

Removed.
    
    -- 
    Fabien.

Thank you!!
doug