Re: dblink: Add SCRAM pass-through authentication

On Mon, Mar 24, 2025 at 1:16 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 21.03.25 19:24, Matheus Alcantara wrote:
> > On Fri, Mar 21, 2025 at 1:28 PM Jacob Champion
> > <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
> >>
> >> Great, thank you! Looking over v10, I think I've run out of feedback
> >> at this point. Marked Ready for Committer.
> >
> > Thanks for all the effort reviewing this patch!
>
> I have committed the 0003 patch (the postgres_fdw bug fix).

Thanks!

> I'm a bit confused about the refactoring patch 0001. There are some
> details there that don't seem right. For example, you write that the
> pfree(rconn) calls are no longer necessary, but AFAICT, it would still
> be needed in dblink_get_conn(). Also, there appear to be some possible
> behavior changes, or at least it's not fully explained, like
> connect_pg_server() doing foreign-server name resolution, which
> dblink_get_conn() did not do before.
>
> But it's actually not clear to me how the refactoring in 0001
> contributes to making the patch 0002 better, since patch 0002 barely
> touches the code touched by 0001.
>
> How would patch 0002 look without 0001 before it? Which code would need
> to be duplicated or what other awkward changes are you trying to avoid?

You are right, I think that the refactor was needed on the initial
versions of the patch because it was referencing the UseScramPassthrough
function in multiple places, so the refactor was needed to accomplish the
parameters of the function.

Since we now assume that the UseScramPassthrough is already checked on
some parts of the code I agree that this refactor is not required
anymore. Attached v11 without the refactor patch.

Thanks!

--
Matheus Alcantara