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

From: Christian Ullrich <chris(at)chrullrich(dot)net>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, Magnus Hagander <magnus(at)hagander(dot)net>, pgsql-committers <pgsql-committers(at)postgresql(dot)org>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pgsql: Add putenv support for msvcrt from Visual Studio 2013
Date: 2016-09-06 08:36:45
Message-ID: 16feb1af-7fbb-49ab-8cf8-f98a6e7da9c0@chrullrich.net
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-committers pgsql-hackers

* Michael Paquier wrote:

> On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich <chris(at)chrullrich(dot)net> wrote:

>> 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?
>
> Looking at the commit logs and 741e4ad7 that does not sound like a good idea.

Well, I still maintain that if it doesn't work and has never worked,
getting rid of it is better than making it work six years after the
fact. OTOH, there may have been cases where it did actually work,
perhaps those gnuwin32 libraries that were mentioned in the comment
before it was changed in that commit above, if they were loaded before
the first call to the function.

OTTH, wouldn't it be funny if fixing it actually broke something that
worked accidentally because it *didn't* get the updated environment? I
think that is at least as likely as suddenly getting excited reports
that something now works that hasn't before.

> It is better to avoid !!. See for example 1a7a436 that avoided
> problems with VS2015 as far as I recall.

Agreed, thanks for noticing. This change creates a warning, however,
because GetModuleHandleEx() returns BOOL, not HMODULE. Updated 0003
attached, simplified over my original one.

> In order to avoid any problems with the load and unload windows, my
> bet goes for 0001, 0002 and 0003, with the last two patches merged
> together, 0001 being only a set of independent fixes. That's ugly, but
> that looks the safest course of actions. I have rebased/rewritten the
> patches as attached. Thoughts?

In lieu of convincing you to drop the entire thing, yes, that looks
about right, except for the BOOL thing.

--
Christian

Attachment Content-Type Size
0003-Pin-any-DLL-as-soon-as-seen-when-looking-for-_putenv.patch text/plain 2.8 KB

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Craig Ringer 2016-09-06 09:01:58 Re: pgsql: Add putenv support for msvcrt from Visual Studio 2013
Previous Message Michael Paquier 2016-09-06 07:11:52 Re: pgsql: Add putenv support for msvcrt from Visual Studio 2013

Browse pgsql-hackers by date

  From Date Subject
Next Message Petr Jelinek 2016-09-06 08:56:53 Re: Proposal for changes to recovery.conf API
Previous Message Aleksander Alekseev 2016-09-06 08:20:27 Re: Suggestions for first contribution?