Обсуждение: be-secure-gssapi.c and auth.c with setenv() not compatible on Windows

Hi all,

Now that hamerkop has been fixed and that we have some coverage with
builds of GSSAPI on Windows thanks to 02511066, the buildfarm has been
complaining about a build failure on Windows for 12 and 13:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=hamerkop&dt=2021-05-28%2011%3A06%3A18&stg=make

The logs are hard to decrypt, but I guess that this is caused by the
use of setenv() in be-secure-gssapi.c and auth.c, as the tree has no
implementation that MSVC could feed on for those branches.

The recent commit 7ca37fb has changed things so as setenv() is used
instead of putenv(), and provides a fallback implementation, which
explains why the compilation of be-secure-gssapi.c and auth.c works
with MSVC, as reported by hamerkop.

We can do two things here:
1) Switch be-secure-gssapi.c and auth.c to use putenv().
2) Backport into 12 and 13 the fallback implementation of setenv
introduced in 7ca37fb, and keep be-secure-gssapi.c as they are now.

It is worth noting that 860fe27 mentions the use of setenv() in
be-secure-gssapi.c but has done nothing for it.  I would choose 1), on
the ground that adding a new file on back-branches adds an additional
cost to Windows maintainers if they use their own MSVC scripts (I know
of one case here that would be impacted), and that does not seem
mandatory here as putenv() would just work.

Thoughts?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> We can do two things here:
> 1) Switch be-secure-gssapi.c and auth.c to use putenv().
> 2) Backport into 12 and 13 the fallback implementation of setenv
> introduced in 7ca37fb, and keep be-secure-gssapi.c as they are now.

There's a lot of value in keeping the branches looking alike.
On the other hand, 7ca37fb hasn't survived contact with the
public yet, so I'm a bit nervous about it.

It's not clear to me how much of 7ca37fb you're envisioning
back-patching in (2).  I think it'd be best to back-patch
only the addition of pgwin32_setenv, and then let the gssapi
code use it.  In that way, if there's anything wrong with
pgwin32_setenv, we're only breaking code that never worked
on Windows before anyway.

            regards, tom lane

On Fri, May 28, 2021 at 11:37:22AM -0400, Tom Lane wrote:
> There's a lot of value in keeping the branches looking alike.
> On the other hand, 7ca37fb hasn't survived contact with the
> public yet, so I'm a bit nervous about it.

I don't think this set of complications is worth the risk
destabilizing those stable branches.

> It's not clear to me how much of 7ca37fb you're envisioning
> back-patching in (2).  I think it'd be best to back-patch
> only the addition of pgwin32_setenv, and then let the gssapi
> code use it.  In that way, if there's anything wrong with
> pgwin32_setenv, we're only breaking code that never worked
> on Windows before anyway.

Just to be clear, for 2) I was thinking to pick up the minimal parts
you have changed in win32env.c and add src/port/setenv.c to add the
fallback implementation of setenv(), without changing anything else.
This also requires grabbing the small changes within pgwin32_putenv(),
visibly.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, May 28, 2021 at 11:37:22AM -0400, Tom Lane wrote:
>> It's not clear to me how much of 7ca37fb you're envisioning
>> back-patching in (2).  I think it'd be best to back-patch
>> only the addition of pgwin32_setenv, and then let the gssapi
>> code use it.  In that way, if there's anything wrong with
>> pgwin32_setenv, we're only breaking code that never worked
>> on Windows before anyway.

> Just to be clear, for 2) I was thinking to pick up the minimal parts
> you have changed in win32env.c and add src/port/setenv.c to add the
> fallback implementation of setenv(), without changing anything else.
> This also requires grabbing the small changes within pgwin32_putenv(),
> visibly.

What I had in mind was to *only* add pgwin32_setenv, not setenv.c.
There's no evidence that any other modern platform lacks setenv.
Moreover, there's no issue in these branches unless your platform
lacks setenv yet has GSS support.

            regards, tom lane

On Sat, May 29, 2021 at 10:44:14AM -0400, Tom Lane wrote:
> What I had in mind was to *only* add pgwin32_setenv, not setenv.c.
> There's no evidence that any other modern platform lacks setenv.
> Moreover, there's no issue in these branches unless your platform
> lacks setenv yet has GSS support.

I have been finally able to poke at that, resulting in the attached.
You are right that adding only the fallback implementation for
setenv() seems to be enough.  I cannot get my environment to complain,
and the code compiles.
--
Michael

On Mon, May 31, 2021 at 09:14:36AM +0900, Michael Paquier wrote:
> I have been finally able to poke at that, resulting in the attached.
> You are right that adding only the fallback implementation for
> setenv() seems to be enough.  I cannot get my environment to complain,
> and the code compiles.

Okay, applied this stuff to 12 and 13 to take care of the build
failures with hamerkop.  The ECPG tests should also turn back to green
there.
--
Michael

On Tue, Jun 01, 2021 at 10:14:49AM +0900, Michael Paquier wrote:
> Okay, applied this stuff to 12 and 13 to take care of the build
> failures with hamerkop.  The ECPG tests should also turn back to green
> there.

hamerkop has reported back, and things are now good on those
branches.  Now for the remaining issue of HEAD..
--
Michael