On Fri Mar 22, 2024 at 12:17 PM CDT, Robert Haas wrote:
> On Fri, Mar 22, 2024 at 1:05 PM Tristan Partin <tristan@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.
This is good feedback, thanks. I have added comments where you
suggested. I reworded the PQsocketPoll docs to hopefully meet your
expectations. I am using the term "connection sequence" which is from
the PQconnectStartParams docs, so hopefully people will be able to make
that connection. I wrote documentation for "forRead" and "forWrite" as
well.
I had a question about parameter naming. Right now I have a mix of
camel-case and snake-case in the function signature since that is what
I inherited. Should I change that to be consistent? If so, which case
would you like?
Thanks for your continued reviews.
--
Tristan Partin
Neon (https://neon.tech)