I'm still not particularly happy with this.  Checking whether I can
polish it up.

a) the new function names are partially non-descriptive and their
   meaning is undocumented.  As an extreme example:

-                               if (!FD_ISSET(sock, &input_mask))
+                               if (ignore_socket(sockets, i, st->con))

   reading the new code it's entirely unclear what that could mean. Are
   you marking the socket as ignored? What does ignored even mean?

   There's not a single comment on what the new functions mean. It's not
   that bad if there's no docs on what FD_ISSET implies, because that's a
   well known and documented interface. But introducing an abstraction
   without any comments on it?

b) Does this actually improve the situation all that much? We still loop
   over every socket:

                /* ok, advance the state machine of each connection */
                for (i = 0; i < nstate; i++)
                        CState     *st = &state[i];

                        if (st->state == CSTATE_WAIT_RESULT)
                                /* don't call doCustom unless data is available 

                                if (error_on_socket(sockets, i, st->con))
                                        goto done;

                                if (ignore_socket(sockets, i, st->con))
                        else if (st->state == CSTATE_FINISHED ||
                                         st->state == CSTATE_ABORTED)
                                /* this client is done, no need to consider it 
anymore */

                        doCustom(thread, st, &aggs);

                        /* If doCustom changed client to finished state, reduce 
remains */
                        if (st->state == CSTATE_FINISHED || st->state == 

   if the goal is to make this more scalable, wouldn't this require
   using a proper polling mechanism that supports signalling the
   sockets with relevant changes, rather than busy-looping through every
   socket if there's just a single change?

   I kinda wonder if the proper fix wouldn't be to have one patch making
   WaitEventSets usable from frontend code, and then make this code use
   them.  Not a small project though.


Andres Freund

Reply via email to