On Thu, Dec 11, 2025 at 11:43 PM Chao Li <li.evan.chao@gmail.com> wrote:
> This is a solid patch set. Only a few small comments:
Thanks for the review!
> The commit message has explained why SOCKTYPE is temporary and the reason why adding prefix “PG_” is to avoid
collisions.But I don’t think code readers will always read commit messages, given the macro is a local and temporary,
whyadding a prefix starting with a underscore, like “_PQ_SOCKTYPE”, which explicitly indicates the macro is kinda
private.
_PQ_SOCKTYPE is reserved (starts with _P), but I could add more
explanatory comments if you think that'd be useful. See v2-0001, which
now includes an explanation of the signature in the documentation.
The hard part is that I don't want to require all Windows clients of
libpq-fe.h to have to depend on Winsock; that's the only reason for
this oddity. Otherwise I'd declare PGsocket as the public version of
our internal pgsocket and call it a day.
> + * Helper for handling user flow failures. If the implementation put anything
> + * into request->error, it's added to conn->errorMessage here.
> ```
>
> Typo: put -> puts
Past tense was my intent, but I've reworded to avoid any garden paths:
"If anything was put into request->error, it's added to
conn->errorMessage here."
> “*/“ in the end of the comment line seems a typo.
Thanks, no idea why I did that.
> "(prefer v2, below, instead)" looks confusing to me, though I can understand what it means. Maybe make it clearer,
like“(v2 is preferred; see below)"
Done.
--
v2 makes these changes and rebases over latest HEAD. I'll plan to get
0001-3 in this week; probably tomorrow.
Open questions remain:
1) 0004: Any objections to putting PQExpBuffer into libpq-fe.h?
2) 0004: Thoughts on the v2 inheritance struct style as opposed to
relying on implementations to double-check the struct length?
3) 0005: Should I add the thread lock to an init() API, or expose a
new PQgetThreadLock() that other code can use?
4) 0007: [all of it]
My personal thoughts on these:
1) it's fine
2) it's a coin flip for me; inheritance is ugly, length magic is scary
3) I like the idea of PQgetThreadLock() so that we don't have to
inject it everywhere it could possibly be needed
Thanks,
--Jacob
