On Fri, Feb 21, 2025 at 6:48 AM Matheus Alcantara <matheusssil...@gmail.com> wrote: > Hi, thanks for all the reviews. Attached v3 with some fixes.
Thanks! I keep getting pulled away from my review of 0002, so I'll just comment on 0001 to get things moving again; sorry for the delay. > I agree, I've just declared outside of get_connect_string because on 0002 we > also need the user mapping for UseScramPassthrough function, so I think that > it > would make the review more easier. Ah, I missed that part of 0002. Works for me. > > > - if (rconn) > > > - pfree(rconn); > > > > Looks like this code in dblink_connect() was dropped. > > > Oops, fixed on connect_pg_server since this logic was moved to this function. I think this fix may break the other usage in dblink_get_conn(), where rconn comes from a hash entry. Maybe dblink_connect() should instead put a PG_CATCH/pfree/PG_RE_THROW around the call to connect_pg_server(), to ensure that the rconn allocation in TopMemoryContext doesn't get leaked? > ## Questions: > > - The new dblink_connstr_has_scam_require_auth function is very similar with > dblink_connstr_has_pw, we may create a common function for these or let it > duplicated? My preference would be to wait for a third [1], but at the very least I think the new function should go right next to the old one, to highlight the similarity. I have attached some stylistic suggestions, plus pgindent/pgperltidy tweaks, as fixup commits 0002 and 0004. Thanks, --Jacob [1] https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)
v4-0001-dblink-refactor-get-connection-routines.patch
Description: Binary data
v4-0002-fixup-dblink-refactor-get-connection-routines.patch
Description: Binary data
v4-0003-dblink-Add-SCRAM-pass-through-authentication.patch
Description: Binary data
v4-0004-fixup-dblink-Add-SCRAM-pass-through-authenticatio.patch
Description: Binary data
