On Fri, Feb 21, 2025 at 6:48 AM Matheus Alcantara
<matheusssilv97@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)
