Re: pgsql: Add putenv support for msvcrt from Visual Studio 2013

* Michael Paquier wrote:

> On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich
<chris(at)chrullrich(dot)net> wrote:

>> * Christian Ullrich wrote:

> And actually, by looking at those patches, isn't it a dangerous
> practice to be able to load multiple versions of the same DLL routines
> in the same workspace? I have personally very bad souvenirs with that,

No, it is exactly what the version-specific CRTs are meant to allow.
Each module uses the CRT version it needs, and they don't share any
state, so absent bugs, they cannot conflict.

Of the processes currently running on my system, 25 have more than one
CRT loaded (one has three, the others two).

> and particularly loading multiple versions of msvcr into the same
> workspace can cause unwanted crashes and corruptions of processes. In
> short I mean this thing: https://en.wikipedia.org/wiki/DLL_Hell.

That was about DLLs existing in different versions with the same file
name, and installers replacing them with their own, leading to problems
with other applications that expected to load their preferred version.
This does not apply to the multiple-CRT situation, because all minor
versions of a given CRT are supposed to be ABI compatible.

> So, shouldn't we first make the DLL list a little bit more severe
> depending on the value of _MSC_VER? I would mean something like that:
> #ifdef _MSC_VER >= 1900
> {"ucrtbase", NULL, NULL},
> #elif _MSC_VER >= 1800
> {"msvcr120", NULL, NULL},
> #elif etc, etc.
> [...]
> #endif
>
> This would require modules to be built using the same msvc version as
> the core server, but that's better than just plain crash if loaded
> DLLs corrupt the stack. Am I missing something?

Yes: This turns (this part of) pgwin32_putenv() into a great big NOP. We
call putenv() anyway on the very last line of the function, so if we
require common-CRT builds, that call alone (together with the
SetEnvironmentVariable() just above) is sufficient.

That said, introducing this requirement would be a very significant
change. I'm not sure how many independently maintained compiled
extensions there are, but this would mean that their developers would
have to have the matching VS versions for every PG distribution they
want to support. Even if that's just EDB, it still is a lot of effort.

My conclusion from April stands:

> The fact that master looks like it does means that there have not been
> many (or any) complaints about missing cross-module environment
> variables. If nobody ever needs to see a variable set elsewhere, we
> have a very simple solution: Why don't we simply throw out the whole
> #ifdef _MSC_VER block?

--
Christian