On Fri, Mar 22, 2024 at 1:05 PM Tristan Partin <tris...@neon.tech> wrote: > Sorry for taking a while to get back to y'all. I have taken your > feedback into consideration for v9. This is my first time writing > Postgres docs, so I'm ready for another round of editing :).
Yeah, that looks like it needs some wordsmithing yet. I can take a crack at that at some point, but here are a few notes: - "takes care of" and "working through the state machine" seem quite vague to me. - the meanings of forRead and forWrite don't seem to be documented. - "retuns" is a typo. > Robert, could you point out some places where comments would be useful > in 0002? I did rename the function and moved it as suggested, thanks! In > turn, I also realized I forgot a prototype, so also added it. Well, for starters, I'd give the function a header comment. Telling me that a 1 second timeout is a 1 second timeout is less useful than telling me why we've picked a 1 second timeout. Presumably the answer is "so we can respond to cancel interrupts in a timely fashion", but I'd make that explicit. It might be worth a comment saying that PQsocket() shouldn't be hoisted out of the loop because it could be a multi-host connection string and the socket might change under us. Unless that's not true, in which case we should hoist the PQsocket() call out of the loop. I think it would also be worth a comment saying that we don't need to report errors here, as the caller will do that; we just need to wait until the connection is known to have either succeeded or failed, or until the user presses cancel. -- Robert Haas EDB: http://www.enterprisedb.com
