Thread

Re: [oauth] Stabilize the libpq-oauth ABI (and allow alternative implementations?)

Jacob Champion <jacob.champion@enterprisedb.com> — 2025-12-16T17:40:44Z

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, why adding 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