Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013
On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich wrote:
> * Michael Paquier wrote:
>
>> On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich
>> 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.
Hm. OK.
> 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.
Yes. FWIW in my stuff everything gets compiled based on the same VS
version and bundled in the same msi, including a set of extensions
compiled from source, but perhaps my sight is too narrow in this
area... Well let's forget about that.
> 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.
+ if (!rtmodules[i].pinned)
+ {
+ HMODULE tmp;
+ BOOL res = GetModuleHandleEx(
+ GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS
+ | GET_MODULE_HANDLE_EX_FLAG_PIN,
+ (LPCTSTR)rtmodules[i].hmodule,
+ &tmp);
+ rtmodules[i].pinned = !!res;
+ }
It is better to avoid !!. See for example 1a7a436 that avoided
problems with VS2015 as far as I recall.
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?
--
Michael
From b809a0b1c323529c2d7460962a3c688ad8aec3f4 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 6 Sep 2016 15:51:55 +0900
Subject: [PATCH 1/3] Cleanups in pgwin32_putenv().
- Added UCRT and all debug CRTs
- Condensed the array to one line per item (it was getting long)
- Test HMODULEs for NULL instead of zero
- Removed unnecessary CloseHandle() call
---
src/port/win32env.c | 61 +
1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index e64065c..77f8334 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -45,36 +45,34 @@ pgwin32_putenv(const char *envval)
PUTENVPROC putenvFunc;
} rtmodules[] =
{
- {
- "msvcrt", 0, NULL
- }, /* Visual Studio 6.0 / mingw */
- {
- "msvcr70", 0, NULL
- }, /* Visual Studio 2002 */
- {
- "msvcr71", 0, NULL
- }, /* Visual Studio 2003 */
- {
- "msvcr80", 0, NULL
- }, /* Visual Studio 2005 */
- {
- "msvcr90", 0, NULL
- }, /* Visual Studio 2008 */
- {
- "msvcr100", 0, NULL
- }, /* Visual Studio 2010 */
- {
- "msvcr110", 0, NULL
- }, /* Visual Studio 2012 */
- {
- "msvcr120", 0, NULL
- }, /* Visual Studio 2013 */
- {
- "ucrtbase", 0, NULL
- }, /* Visual Studio 2015 and later */
- {
- NULL, 0, NULL
- }
+ /* Visual Studio 6.0 / mingw */
+ {"msvcrt", NULL, NULL},
+ {"msvcrtd", NULL, NULL},
+ /* Visual Studio 2002 */
+ {"msvcr70", NULL, NULL},
+ {"msvcr70d", NULL, NULL},
+ /* Visual Studio 2003 */
+ {"msvcr71", NULL, NULL},
+ {"msvcr71d", NULL, NULL},
+ /* Visual Studio 2005 */
+ {"msvcr80", NULL, NULL},
+ {"msvcr80d", NULL, NULL},
+ /* Visual Studio 2008 */
+ {"msvcr90", NULL, NULL},
+ {"msvcr90d", NULL, NULL},
+ /* Visual Studio 2010 */
+ {"msvcr100", NULL, NULL},
+ {"msvcr100d", NULL, NULL},
+ /* Visual Studio 2012 */
+ {"msvcr110", NULL, NULL},
+ {"msvcr110d", NULL, NULL},
+ /* Visual Studio 2013 */
+ {"msvcr120", NULL, NULL},
+ {"msvcr120d", NULL, NULL},
+ /* Visual Studio 2015 and later */
+ {"ucrtbase", NULL, NULL},
+ {"ucrtbased", NULL, NULL},
+ {NULL, NULL, NULL}
};
int i;
@@ -82,7 +80,7 @@ pgwin32_putenv(const char *envval)
{
if (rtmodules[i].putenvFunc == NULL)
{
- if (rtmodules[i].hmodule == 0)
+ if (rtmodules[i].hmodule == NULL)
{
/* Not attempted before, so try to find this DLL */
rtmodules[i].hmodule = GetModuleHan
Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013
* Michael Paquier wrote:
On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich 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
>From dfbe7384b309c20d7733b0d18e6819302a483d95 Mon Sep 17 00:00:00 2001
From: Christian Ullrich
Date: Tue, 6 Sep 2016 10:02:06 +0200
Subject: [PATCH] Pin any DLL as soon as seen when looking for _putenv()
---
src/port/win32env.c | 54 ++---
1 file changed, 35 insertions(+), 19 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 3f56ba8..7417b60 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -43,36 +43,37 @@ pgwin32_putenv(const char *envval)
char *modulename;
HMODULE hmodule;
PUTENVPROC putenvFunc;
+ BOOLpinned;
} rtmodules[] =
{
/* Visual Studio 6.0 / mingw */
- {"msvcrt", NULL, NULL},
- {"msvcrtd", NULL, NULL},
+ {"msvcrt", NULL, NULL, FALSE},
+ {"msvcrtd", NULL, NULL, FALSE},
/* Visual Studio 2002 */
- {"msvcr70", NULL, NULL},
- {"msvcr70d",NULL, NULL},
+ {"msvcr70", NULL, NULL, FALSE},
+ {"msvcr70d",NULL, NULL, FALSE},
/* Visual Studio 2003 */
- {"msvcr71", NULL, NULL},
- {"msvcr71d",NULL, NULL},
+ {"msvcr71", NULL, NULL, FALSE},
+ {"msvcr71d",NULL, NULL, FALSE},
/* Visual Studio 2005 */
- {"msvcr80", NULL, NULL},
- {"msvcr80d",NULL, NULL},
+ {"msvcr80", NULL, NULL, FALSE},
+ {"msvcr80d",NULL, NULL, FALSE},
/* Visual Studio 2008 */
- {"msvcr90", NULL, NULL},
- {"msvcr90d",NULL, NULL},
+ {"msvcr90", NULL, NULL, FALSE},
+ {"msvcr90d",NULL, NULL, FALSE},
/* Visual Studio 2010 */
- {"msvcr100",NULL, NULL},
- {"msvcr100d", NULL, NULL},
+ {"msvcr100",NULL, NULL, FALSE},
+ {"msvcr100d", NULL, NULL, FALSE},
/* Visual Studio 2012 */
- {"msvcr110",NULL, NULL},
- {"msvcr110d", NULL, NULL},
+ {"msvcr110",NULL, NULL, FALSE},
+ {"msvcr110d", NULL, NULL, FALSE},
/* Visual Studio 2013 */
- {"msvcr120",NULL, NULL},
- {"msvcr120d", NULL, NULL},
+ {"msvcr120",NULL, NULL, FALSE},
+ {"msvcr120d", NULL, NULL, FALSE},
/* Visual Studio 2015 and later */
- {"ucrtbase",NULL, NULL},
- {"ucrtbased", NULL, NULL},
- {NULL, NULL, NULL}
+ {"ucrtbase",NULL, NULL, FALSE},
+ {"ucrtbased", NULL, N
Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013
On 6 Sep. 2016 15:12, "Michael Paquier" wrote: > > On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich wrote: > > > 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. > > Yes. FWIW in my stuff everything gets compiled based on the same VS > version and bundled in the same msi, including a set of extensions > compiled from source, but perhaps my sight is too narrow in this > area... Well let's forget about that. 3rd party extensions may not and may not be able to. Most obvious example is people building things with mingw. This is just expected to work on win32. Breaking this assumption will cause pain. Requiring a single unified C runtime across the process isn't viable. It isn't like Unix. You might have a legacy DLL compiled with Borland C that you're wrapping up to expose as an extension using mingw to link into a Pg compiled with MSVC.
Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013
On Tue, Sep 6, 2016 at 5:36 PM, Christian Ullrich wrote: > * Michael Paquier wrote: >> 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. Yes, right. Thanks. Patch 0001 is definitely something that should be applied and backpatched, the CloseHandle() call is buggy. Now 0002 and 0003 are improving things, but there have been no reports regarding problems in this area, so this could just be applied to master I guess. Christian, does that sound right? By the way, how is it decided that a DLL gets unloaded in a process if it is not pinned? Is that something the OS decides? -- Michael -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013
* Michael Paquier wrote: On Tue, Sep 6, 2016 at 5:36 PM, Christian Ullrich wrote: * Michael Paquier wrote: 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. Yes, right. Thanks. Patch 0001 is definitely something that should be applied and backpatched, the CloseHandle() call is buggy. Now 0002 and 0003 are improving things, but there have been no reports regarding problems in this area, so this could just be applied to master I guess. Christian, does that sound right? Yes. By the way, how is it decided that a DLL gets unloaded in a process if it is not pinned? Is that something the OS decides? Reference counting in LoadLibrary() and FreeLibrary(), among other places. For example, GetModuleHandleEx() (but not the non-Ex) will by default increment the counter. -- Christian -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Fix VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL
Fix VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL lazy_truncate_heap() was waiting for VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL, but in microseconds not milliseconds as originally intended. Found by code inspection. Simon Riggs Branch -- master Details --- http://git.postgresql.org/pg/commitdiff/dcb12ce8d8691e0e526d3f38d14c4d7fc9c664f5 Modified Files -- src/backend/commands/vacuumlazy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Repair whitespace in initdb message.
Repair whitespace in initdb message. What used to be four spaces somehow turned into a tab and a couple of spaces in commit a00c58314, no doubt from overhelpful emacs autoindent. Noted by Peter Eisentraut. Branch -- master Details --- http://git.postgresql.org/pg/commitdiff/a2ee579b6def8e0bde876c6c2fc9d4b8ec2b6b67 Modified Files -- src/bin/initdb/initdb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Teach appendShellString() to not quote strings containing "-".
Teach appendShellString() to not quote strings containing "-". Brain fade in commit a00c58314: I was thinking that a string starting with "-" could be taken as a switch depending on command line syntax. That's true, but having appendShellString() quote it will not help, so we may as well not do so. Per complaint from Peter Eisentraut. Branch -- master Details --- http://git.postgresql.org/pg/commitdiff/cdc70597c9ba62aad08a46e55c0c783bf4c21c9c Modified Files -- src/fe_utils/string_utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Guard against possible memory allocation botch in batchmemtuples
Guard against possible memory allocation botch in batchmemtuples(). Negative availMemLessRefund would be problematic. It's not entirely clear whether the case can be hit in the code as it stands, but this seems like good future-proofing in any case. While we're at it, insist that the value be not merely positive but not tiny, so as to avoid doing a lot of repalloc work for little gain. Peter Geoghegan Discussion: Branch -- REL9_6_STABLE Details --- http://git.postgresql.org/pg/commitdiff/96ba40c0f15aa1e950b35536387fde30ebbc4547 Modified Files -- src/backend/utils/sort/tuplesort.c | 23 +-- 1 file changed, 21 insertions(+), 2 deletions(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Guard against possible memory allocation botch in batchmemtuples
Guard against possible memory allocation botch in batchmemtuples(). Negative availMemLessRefund would be problematic. It's not entirely clear whether the case can be hit in the code as it stands, but this seems like good future-proofing in any case. While we're at it, insist that the value be not merely positive but not tiny, so as to avoid doing a lot of repalloc work for little gain. Peter Geoghegan Discussion: Branch -- master Details --- http://git.postgresql.org/pg/commitdiff/f032722f86a709277d44a317a3bc8acd74861a72 Modified Files -- src/backend/utils/sort/tuplesort.c | 23 +-- 1 file changed, 21 insertions(+), 2 deletions(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Doc: small improvements for documentation about VACUUM freezing.
Doc: small improvements for documentation about VACUUM freezing. Mostly, explain how row xmin's used to be replaced by FrozenTransactionId and no longer are. Do a little copy-editing on the side. Per discussion with Egor Rogov. Back-patch to 9.4 where the behavioral change occurred. Discussion: <[email protected]> Branch -- REL9_5_STABLE Details --- http://git.postgresql.org/pg/commitdiff/6cdc26e482182a872d10a899cc4bbdb11364720b Modified Files -- doc/src/sgml/maintenance.sgml | 42 -- 1 file changed, 32 insertions(+), 10 deletions(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Doc: small improvements for documentation about VACUUM freezing.
Doc: small improvements for documentation about VACUUM freezing. Mostly, explain how row xmin's used to be replaced by FrozenTransactionId and no longer are. Do a little copy-editing on the side. Per discussion with Egor Rogov. Back-patch to 9.4 where the behavioral change occurred. Discussion: <[email protected]> Branch -- REL9_6_STABLE Details --- http://git.postgresql.org/pg/commitdiff/ed3598f33f4c2a9eb07678bd29a90919e6c63938 Modified Files -- doc/src/sgml/maintenance.sgml | 48 +++ 1 file changed, 35 insertions(+), 13 deletions(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Doc: small improvements for documentation about VACUUM freezing.
Doc: small improvements for documentation about VACUUM freezing. Mostly, explain how row xmin's used to be replaced by FrozenTransactionId and no longer are. Do a little copy-editing on the side. Per discussion with Egor Rogov. Back-patch to 9.4 where the behavioral change occurred. Discussion: <[email protected]> Branch -- REL9_4_STABLE Details --- http://git.postgresql.org/pg/commitdiff/db88112ce17f05b211cf6e2c50f1dd7107556786 Modified Files -- doc/src/sgml/maintenance.sgml | 42 -- 1 file changed, 32 insertions(+), 10 deletions(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Doc: small improvements for documentation about VACUUM freezing.
Doc: small improvements for documentation about VACUUM freezing. Mostly, explain how row xmin's used to be replaced by FrozenTransactionId and no longer are. Do a little copy-editing on the side. Per discussion with Egor Rogov. Back-patch to 9.4 where the behavioral change occurred. Discussion: <[email protected]> Branch -- master Details --- http://git.postgresql.org/pg/commitdiff/975768f8eae2581b89ceafe8b16a77ff375207fe Modified Files -- doc/src/sgml/maintenance.sgml | 48 +++ 1 file changed, 35 insertions(+), 13 deletions(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Add location field to DefElem
Add location field to DefElem Add a location field to the DefElem struct, used to parse many utility commands. Update various error messages to supply error position information. To propogate the error position information in a more systematic way, create a ParseState in standard_ProcessUtility() and pass that to interested functions implementing the utility commands. This seems better than passing the query string and then reassembling a parse state ad hoc, which violates the encapsulation of the ParseState type. Reviewed-by: Pavel Stehule Branch -- master Details --- http://git.postgresql.org/pg/commitdiff/49eb0fd0972d14014dd3533b1f1bf8c94c899883 Modified Files -- contrib/file_fdw/file_fdw.c| 16 +- src/backend/access/common/reloptions.c | 2 +- src/backend/catalog/aclchk.c | 8 +- src/backend/commands/aggregatecmds.c | 7 +- src/backend/commands/collationcmds.c | 5 +- src/backend/commands/copy.c| 93 ++ src/backend/commands/dbcommands.c | 61 +++--- src/backend/commands/define.c | 9 - src/backend/commands/explain.c | 8 +- src/backend/commands/extension.c | 25 ++- src/backend/commands/functioncmds.c| 57 +++--- src/backend/commands/sequence.c| 36 ++-- src/backend/commands/tsearchcmds.c | 8 +- src/backend/commands/typecmds.c| 8 +- src/backend/commands/user.c| 41 ++-- src/backend/commands/view.c| 4 +- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c | 2 + src/backend/nodes/makefuncs.c | 6 +- src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/parser/gram.y | 248 - src/backend/parser/parse_utilcmd.c | 5 +- src/backend/replication/logical/logicalfuncs.c | 2 +- src/backend/replication/repl_gram.y| 16 +- src/backend/tcop/utility.c | 64 --- src/include/commands/collationcmds.h | 2 +- src/include/commands/copy.h| 7 +- src/include/commands/dbcommands.h | 4 +- src/include/commands/defrem.h | 13 +- src/include/commands/explain.h | 3 +- src/include/commands/extension.h | 4 +- src/include/commands/sequence.h| 5 +- src/include/commands/typecmds.h| 2 +- src/include/commands/user.h| 3 +- src/include/nodes/makefuncs.h | 4 +- src/include/nodes/parsenodes.h | 1 + src/include/utils/acl.h| 3 +- 38 files changed, 438 insertions(+), 347 deletions(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
[COMMITTERS] pgsql: Add a HINT for client-vs-server COPY failure cases.
Add a HINT for client-vs-server COPY failure cases. Users often get confused between COPY and \copy and try to use client-side paths with COPY. The server then cannot find the file (if remote), or sees a permissions problem (if local), or some variant of that. Emit a hint about this in the most common cases. In future we might want to expand the set of errnos for which the hint gets printed, but be conservative for now. Craig Ringer, reviewed by Christoph Berg and Tom Lane Discussion: Branch -- master Details --- http://git.postgresql.org/pg/commitdiff/4f405c8ef4704b7fa7fd8ac14a66c5f5d13722c4 Modified Files -- src/backend/commands/copy.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) -- Sent via pgsql-committers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
