Dear Katsuragi-san, Thank you for reviewing! PSA new version.
> 0001: > Extending pqSocketPoll seems to be a better way because we can > avoid having multiple similar functions. I also would like to hear > horiguchi-san's opinion whether this matches his expectation. > Improvements of pqSocketPoll/pqSocketCheck is discussed in this > thread[1]. I'm concerned with the discussion. I checked the thread and seems correct. I can post +1 to the thread. And the modification will be automatically reflected to the feature if we use the same function, I thought. > As for the function's name, what do you think about keeping > current name (pqSocketCheck)? pqSocketIsReadable... describes > the functionality very well though. No objection, I can keep the shorter name. > pqConnCheck seems to be a family of pqReadReady or pqWriteRedy, > so how about placing pqConnCheck below them? Moved. > + * Moreover, when neither forRead nor forWrite is requested and timeout > is > + * disabled, try to check the health of socket. > Isn't it better to put the comment on how the arguments are > interpreted before the description of return value? > > +#if defined(POLLRDHUP) > + input_fd.events = POLLRDHUP | POLLHUP | > POLLNVAL; > ... > + input_fd.events |= POLLERR; > To my understanding, POLLHUP, POLLNVAL and POLLERR are ignored > in event. Are they necessary? I read man poll(3) again, and I found that these event is ignored when it sets to the events attribute. So removed. > 0002: > As for the return value of postgres_fdw_verify_connection_states, > what do you think about returning NULL when connection-checking > is not performed? I think there are two cases 1) ConnectionHash > is not initialized or 2) connection is not found for specified > server name, That is, no entry passes the first if statement below > (case 2)). > > ``` > if (all || entry->serverid == serverid) > { > if (PQconnCheck(entry->conn)) > { > ``` I think in that case we can follow postgres_fdw_disconnect(). About postgres_fdw_disconnect(), if the given server_name does not exist, an error is reported. This is a current behavior so I want to keep it. Besides, I added the description to document. > 0004: > I'm wondering if we should add kqueue support in this version, > because adding kqueue support introduces additional things to > be considered. What do you think about focusing on the main > functionality using poll in this patch and going for kqueue > support after this patch? I think it is better because it can keep patches smaller. So I stopped attaching 0004. Best Regards, Hayato Kuroda FUJITSU LIMITED
v32-0001-Add-PQconnCheck-and-PQconnCheckable-to-libpq.patch
Description: v32-0001-Add-PQconnCheck-and-PQconnCheckable-to-libpq.patch
v32-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
Description: v32-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
v32-0003-add-test.patch
Description: v32-0003-add-test.patch