Horton

Thread

Commits

  1. Fix for globals.c- c.h must come first

  2. Allow group access on PGDATA

  3. Refactor dir/file permissions

  1. Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Mark Dilger <hornschnorter@gmail.com> — 2018-05-15T15:33:26Z

    Hackers,

There was a bug report sent by Hao Lee about Windows build breakage,
"BUG #15167: error C2365: 'errcode' : redefinition; previous definition"
https://www.postgresql.org/message-id/152446498404.19807.4659286570762153837%40wrigleys.postgresql.org

Heikki was the only person who responded to Hao, AFAIK, though that
conversation did not go far.  Perhaps Heikki was unable to reproduce?  

Hao did not specify which commit caused the problem, so I started
bisecting to find out.

With my Windows build setup, building from master, the builds break
staring with da9b580d89903fee871cf54845ffa2b26bda2e11 committed by
Stephen on Apr 7 2018.



Build FAILED.

"D:\jenkins\workspace\trunk\dist\msi\BUILD\postgresql\pgsql.sln" (default target) (1) ->
(postgres target) -> 
  .\src\backend\replication\basebackup.c(1470): warning C4146: unary minus operator applied to unsigned type, result still unsigned


"D:\jenkins\workspace\trunk\dist\msi\BUILD\postgresql\pgsql.sln" (default target) (1) ->
(postgres target) -> 
  .\src\common\file_perm.c(18): error C2065: 'S_IRWXU' : undeclared identifier
  .\src\common\file_perm.c(18): error C2099: initializer is not a constant
  .\src\common\file_perm.c(19): error C2065: 'S_IRUSR' : undeclared identifier
  .\src\common\file_perm.c(19): error C2065: 'S_IWUSR' : undeclared identifier
  .\src\common\file_perm.c(19): error C2099: initializer is not a constant



The build breaks in a different way staring with
c37b3d08ca6873f9d4eaf24c72a90a550970cbb8 committed by Stephen later
that same day.



Microsoft (R) Build Engine Version 3.5.30729.5420
[Microsoft .NET Framework, Version 2.0.50727.8784]
Copyright (C) Microsoft Corporation 2007. All rights reserved.

Build started 5/15/2018 5:24:08 AM.
Project "D:\jenkins\workspace\trunk\dist\msi\BUILD\postgresql\pgsql.sln" on node 0 (default targets).
 Building solution configuration "Release|x64".
<http://aws-pmc-build-w2k8.int.port25.com:8282/job/trunk/ws/dist/msi/build/postgresql/src/include/utils/elog.h(131)>: error C2365: 'errcode' : redefinition; previous definition was 'typedef'
Done Building Project "D:\jenkins\workspace\trunk\dist\msi\BUILD\postgresql\pgsql.sln" (default targets) -- FAILED.

Build FAILED.



Since I am able to reproduce the problem, I'd like to help debug the
problem.  I find it frustrating that the compiler is not specifying
*where* the prior typedef comes from.

My best guess at the moment is:

diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index c1f0441b08..0a3163398f 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -16,8 +16,11 @@
  *
  *-------------------------------------------------------------------------
  */
+#include <sys/stat.h>
+
 #include "postgres.h"
 
+#include "common/file_perm.h"
 #include "libpq/libpq-be.h"
 #include "libpq/pqcomm.h"
 #include "miscadmin.h"

Which makes me wonder if <sys/stat.h> on Windows declares a typedef for errcode?
The work-around in src/include/port/win32.h which attempts to deal with system
headers defining errcode looks like it won't work unless it gets included *before*
the offending system header, which appears not to be true for globals.c.

Indeed, the following change (shown here for illustrative purposes only; please
don't commit it this way) fixes the problem, at least in my build environment:

diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 9f1209323a..1622b0be62 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -16,7 +16,14 @@
  *
  *-------------------------------------------------------------------------
  */
+
+#if defined(_WIN32) || defined(WIN32) || defined(_MSC_VER) || defined(HAVE_CRTDEFS_H)
+#define errcode __msvc_errcode
+#include <sys/stat.h>
+#undef errcode
+#else
 #include <sys/stat.h>
+#endif
 
 #include "postgres.h"
 


Let me know if there are any tests you'd like me to perform to further
investigate.


mark

  2. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-15T15:58:04Z

    Mark Dilger <hornschnorter@gmail.com> writes:
> My best guess at the moment is:

> diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
> index c1f0441b08..0a3163398f 100644
> --- a/src/backend/utils/init/globals.c
> +++ b/src/backend/utils/init/globals.c
> @@ -16,8 +16,11 @@
>   *
>   *-------------------------------------------------------------------------
>   */
> +#include <sys/stat.h>
> +
>  #include "postgres.h"
 
> +#include "common/file_perm.h"

Yipes.  Frost, you didn't really do that did you?  That's a blatant
break of the "c.h must come first" rule.  Whether or not it broke the
Windows build, there are other platforms it'll break.

> Indeed, the following change (shown here for illustrative purposes only; please
> don't commit it this way) fixes the problem, at least in my build environment:

That's pretty ugly, but what happens if you just move the <sys/stat.h>
inclusion to immediately after postgres.h, as is our normal custom?

			regards, tom lane

  3. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Mark Dilger <hornschnorter@gmail.com> — 2018-05-15T16:27:30Z

    > On May 15, 2018, at 8:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnorter@gmail.com> writes:
>> My best guess at the moment is:
> 
>> diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
>> index c1f0441b08..0a3163398f 100644
>> --- a/src/backend/utils/init/globals.c
>> +++ b/src/backend/utils/init/globals.c
>> @@ -16,8 +16,11 @@
>>  *
>>  *-------------------------------------------------------------------------
>>  */
>> +#include <sys/stat.h>
>> +
>> #include "postgres.h"
> 
>> +#include "common/file_perm.h"
> 
> Yipes.  Frost, you didn't really do that did you?  That's a blatant
> break of the "c.h must come first" rule.  Whether or not it broke the
> Windows build, there are other platforms it'll break.
> 
>> Indeed, the following change (shown here for illustrative purposes only; please
>> don't commit it this way) fixes the problem, at least in my build environment:
> 
> That's pretty ugly, but what happens if you just move the <sys/stat.h>
> inclusion to immediately after postgres.h, as is our normal custom?

That also works.

mark

  4. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Stephen Frost <sfrost@snowman.net> — 2018-05-15T16:29:23Z

    Greetings,

* Mark Dilger (hornschnorter@gmail.com) wrote:
> > On May 15, 2018, at 8:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Mark Dilger <hornschnorter@gmail.com> writes:
> >> My best guess at the moment is:
> > 
> >> diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
> >> index c1f0441b08..0a3163398f 100644
> >> --- a/src/backend/utils/init/globals.c
> >> +++ b/src/backend/utils/init/globals.c
> >> @@ -16,8 +16,11 @@
> >>  *
> >>  *-------------------------------------------------------------------------
> >>  */
> >> +#include <sys/stat.h>
> >> +
> >> #include "postgres.h"
> > 
> >> +#include "common/file_perm.h"
> > 
> > Yipes.  Frost, you didn't really do that did you?  That's a blatant
> > break of the "c.h must come first" rule.  Whether or not it broke the
> > Windows build, there are other platforms it'll break.

Evidently I managed to.

> >> Indeed, the following change (shown here for illustrative purposes only; please
> >> don't commit it this way) fixes the problem, at least in my build environment:
> > 
> > That's pretty ugly, but what happens if you just move the <sys/stat.h>
> > inclusion to immediately after postgres.h, as is our normal custom?
> 
> That also works.

Good, will fix.

Thanks!

Stephen

  5. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Mark Dilger <hornschnorter@gmail.com> — 2018-05-15T16:39:23Z

    > On May 15, 2018, at 9:29 AM, Stephen Frost <sfrost@snowman.net> wrote:
> 
> Greetings,
> 
> * Mark Dilger (hornschnorter@gmail.com) wrote:
>>> On May 15, 2018, at 8:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Mark Dilger <hornschnorter@gmail.com> writes:
>>>> My best guess at the moment is:
>>> 
>>>> diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
>>>> index c1f0441b08..0a3163398f 100644
>>>> --- a/src/backend/utils/init/globals.c
>>>> +++ b/src/backend/utils/init/globals.c
>>>> @@ -16,8 +16,11 @@
>>>> *
>>>> *-------------------------------------------------------------------------
>>>> */
>>>> +#include <sys/stat.h>
>>>> +
>>>> #include "postgres.h"
>>> 
>>>> +#include "common/file_perm.h"
>>> 
>>> Yipes.  Frost, you didn't really do that did you?  That's a blatant
>>> break of the "c.h must come first" rule.  Whether or not it broke the
>>> Windows build, there are other platforms it'll break.
> 
> Evidently I managed to.
> 
>>>> Indeed, the following change (shown here for illustrative purposes only; please
>>>> don't commit it this way) fixes the problem, at least in my build environment:
>>> 
>>> That's pretty ugly, but what happens if you just move the <sys/stat.h>
>>> inclusion to immediately after postgres.h, as is our normal custom?
>> 
>> That also works.
> 
> Good, will fix.

I'm curious why the Windows build farm members did not pick this up.  Or
perhaps they did?  (I don't get emails about that.)  If none of the animals
are configured to detect this bug, perhaps the community needs another
Windows animal configured along the lines of the build machine I am using?

Please advise...

mark

  6. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-15T16:54:05Z

    Mark Dilger <hornschnorter@gmail.com> writes:
> I'm curious why the Windows build farm members did not pick this up.  Or
> perhaps they did?  (I don't get emails about that.)

They did not, and I too was wondering why not.

> If none of the animals
> are configured to detect this bug, perhaps the community needs another
> Windows animal configured along the lines of the build machine I am using?

+1.  How do you have yours configured, anyway?

			regards, tom lane

  7. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Mark Dilger <hornschnorter@gmail.com> — 2018-05-15T17:08:24Z

    > On May 15, 2018, at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnorter@gmail.com> writes:
>> I'm curious why the Windows build farm members did not pick this up.  Or
>> perhaps they did?  (I don't get emails about that.)
> 
> They did not, and I too was wondering why not.
> 
>> If none of the animals
>> are configured to detect this bug, perhaps the community needs another
>> Windows animal configured along the lines of the build machine I am using?
> 
> +1.  How do you have yours configured, anyway?

I mostly develop on mac and linux and don't look at the windows system
too much:


Windows Server 2008 R2 Standard
Service Pack 1
Processor: Intel Xeon CPU E5-2676 v3 @ 2.40GHz
Installed memory:  8.00 GB
System type: 64 bit Operating System

Microsoft Visual Studio 2008 Beta2 x64 cross tools
Microsoft (R) C/C++ Optimizing Compiler Version 15.00.21022.08 for x64

  8. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-15T17:20:05Z

    Mark Dilger <hornschnorter@gmail.com> writes:
>>> If none of the animals
>>> are configured to detect this bug, perhaps the community needs another
>>> Windows animal configured along the lines of the build machine I am using?

>> +1.  How do you have yours configured, anyway?

> I mostly develop on mac and linux and don't look at the windows system
> too much:

> Windows Server 2008 R2 Standard
> Service Pack 1
> Microsoft Visual Studio 2008 Beta2 x64 cross tools

Hm.  I'm not sure what our nominal support range for Visual Studio is.
I see that mastodon (running VS2005) and currawong (running VS2008)
haven't reported on HEAD lately, and I think they may have been shut
down intentionally due to desupport?  But if your build still works
then it seems like we could continue to support VS2008.

			regards, tom lane

  9. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Mark Dilger <hornschnorter@gmail.com> — 2018-05-15T17:22:11Z

    > On May 15, 2018, at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnorter@gmail.com> writes:
>>>> If none of the animals
>>>> are configured to detect this bug, perhaps the community needs another
>>>> Windows animal configured along the lines of the build machine I am using?
> 
>>> +1.  How do you have yours configured, anyway?
> 
>> I mostly develop on mac and linux and don't look at the windows system
>> too much:
> 
>> Windows Server 2008 R2 Standard
>> Service Pack 1
>> Microsoft Visual Studio 2008 Beta2 x64 cross tools
> 
> Hm.  I'm not sure what our nominal support range for Visual Studio is.
> I see that mastodon (running VS2005) and currawong (running VS2008)
> haven't reported on HEAD lately, and I think they may have been shut
> down intentionally due to desupport?  But if your build still works
> then it seems like we could continue to support VS2008.

I don't have a strong opinion on that.  I could also look to upgrade
to a newer version.  Generally, I try to build using the oldest
supported version rather than the newest.  What is the next oldest
working test machine you have?

mark

  10. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-15T17:29:24Z

    Mark Dilger <hornschnorter@gmail.com> writes:
>> On May 15, 2018, at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm.  I'm not sure what our nominal support range for Visual Studio is.
>> I see that mastodon (running VS2005) and currawong (running VS2008)
>> haven't reported on HEAD lately, and I think they may have been shut
>> down intentionally due to desupport?  But if your build still works
>> then it seems like we could continue to support VS2008.

> I don't have a strong opinion on that.  I could also look to upgrade
> to a newer version.  Generally, I try to build using the oldest
> supported version rather than the newest.  What is the next oldest
> working test machine you have?

thrips is described as running VS2010, though I'm not sure how to verify
that it's not been updated.  The make log shows

  c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\bin\AMD64\CL.exe

but is "10.0" the same thing as "2010"?

			regards, tom lane

  11. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Andrew Dunstan <andrew@dunslane.net> — 2018-05-15T17:45:12Z

    
On 05/15/2018 01:20 PM, Tom Lane wrote:
> Mark Dilger <hornschnorter@gmail.com> writes:
>>>> If none of the animals
>>>> are configured to detect this bug, perhaps the community needs another
>>>> Windows animal configured along the lines of the build machine I am using?
>>> +1.  How do you have yours configured, anyway?
>> I mostly develop on mac and linux and don't look at the windows system
>> too much:
>> Windows Server 2008 R2 Standard
>> Service Pack 1
>> Microsoft Visual Studio 2008 Beta2 x64 cross tools
> Hm.  I'm not sure what our nominal support range for Visual Studio is.
> I see that mastodon (running VS2005) and currawong (running VS2008)
> haven't reported on HEAD lately, and I think they may have been shut
> down intentionally due to desupport?  But if your build still works
> then it seems like we could continue to support VS2008.
>
> 			


currawong and friends were shut down in January on >= 11 because of the 
huge pages issue on 32-bit XP. So they aren't going to be revived. Maybe 
we can make VS2008 work on a 64 bit machine. Personally I'm more 
interested in better getting coverage for the modern compilers.


cheers

andrew

  12. Re: Windows build broken starting at da9b580d89903fee871cf54845ffa2b26bda2e11

    Pantelis Theodosiou <ypercube@gmail.com> — 2018-05-15T18:50:40Z

    On Tue, May 15, 2018 at 6:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Mark Dilger <hornschnorter@gmail.com> writes:
>
> > I don't have a strong opinion on that.  I could also look to upgrade
> > to a newer version.  Generally, I try to build using the oldest
> > supported version rather than the newest.  What is the next oldest
> > working test machine you have?
>
> thrips is described as running VS2010, though I'm not sure how to verify
> that it's not been updated.  The make log shows
>
>   c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\bin\AMD64\CL.exe
>
> but is "10.0" the same thing as "2010"?
>
>                         regards, tom lane
>
>
According to Wikipedia, yes:
https://en.wikipedia.org/wiki/Microsoft_Visual_Studio

(although the 10s are only by coincidence, 11.0 is VS 2012, ...)