Thread
Commits
-
Quieten warnings about unused variables
- 28ae524bbf86 11.0 landed
-
Windows warnings from VS 2017
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-09-20T22:08:58Z
I'm working through testing the changes to allow building with Visual Studio 2017. I've noticed that there are some warnings that we don't see in earlier compilers. First, it warns about a couple of unused variables at lines 4553 and 4673 of src/backend/optimizer/path/costsize.c. I think we can do a little rearrangement to keep it happy there. It's also warning that it will copy 16 bytes to a 13 byte structure at lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't seen any ill effects of this so far, but it seems to indicate that something is possibly amiss on this compiler with the MemSet macros. The regression tests are currently failing on my test platform (Windows Server 2016) because it says it can't change permissions on the testtablespace directory. I have no idea what's going on there, still investigating. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Windows warnings from VS 2017
Michael Paquier <michael.paquier@gmail.com> — 2017-09-20T22:13:02Z
On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > First, it warns about a couple of unused variables at lines 4553 and > 4673 of src/backend/optimizer/path/costsize.c. I think we can do a > little rearrangement to keep it happy there. Those are around for some time, see here: https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com But there has been no actual agreement about how to fix them.. -- Michael
-
Re: Windows warnings from VS 2017
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-09-20T22:49:10Z
On 09/20/2017 06:13 PM, Michael Paquier wrote: > On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan > <andrew.dunstan@2ndquadrant.com> wrote: >> First, it warns about a couple of unused variables at lines 4553 and >> 4673 of src/backend/optimizer/path/costsize.c. I think we can do a >> little rearrangement to keep it happy there. > Those are around for some time, see here: > https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com > But there has been no actual agreement about how to fix them.. Oh. Missed that. My solution was going to be slightly different. I was going to enclose the #ifdef'd code in a bare block and move the rte declaration inside that block. I think the difference between these cases and other cases of the PG_USER_FOR_ASSERTS_ONLY macro is that these variables are only even written to by assert-enabled code, whereas in the other cases the variable is written to by non-assert-enabed code but only read by assert-enabled code. The Microsoft compilers don't seem to mind that so much. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Windows warnings from VS 2017
Michael Paquier <michael.paquier@gmail.com> — 2017-09-20T23:27:15Z
On Thu, Sep 21, 2017 at 7:49 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > On 09/20/2017 06:13 PM, Michael Paquier wrote: >> On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan >> <andrew.dunstan@2ndquadrant.com> wrote: >>> First, it warns about a couple of unused variables at lines 4553 and >>> 4673 of src/backend/optimizer/path/costsize.c. I think we can do a >>> little rearrangement to keep it happy there. >> Those are around for some time, see here: >> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com >> But there has been no actual agreement about how to fix them.. > > Oh. Missed that. > > My solution was going to be slightly different. I was going to enclose > the #ifdef'd code in a bare block and move the rte declaration inside > that block. That looks like a fine approach to me. -- Michael
-
Re: Windows warnings from VS 2017
Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-20T23:32:02Z
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > On 09/20/2017 06:13 PM, Michael Paquier wrote: >> Those are around for some time, see here: >> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com >> But there has been no actual agreement about how to fix them.. > Oh. Missed that. > My solution was going to be slightly different. I was going to enclose > the #ifdef'd code in a bare block and move the rte declaration inside > that block. Of the various solutions proposed in the previous thread, I think the most salable alternative is probably ilmari's: get rid of the variable and write the assert like Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY); That's a pretty minimal change and it doesn't add any cycles to the non-Assert case. regards, tom lane
-
Re: Windows warnings from VS 2017
Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-20T23:54:54Z
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > It's also warning that it will copy 16 bytes to a 13 byte structure at > lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't > seen any ill effects of this so far, but it seems to indicate that > something is possibly amiss on this compiler with the MemSet macros. That's weird. Is it too stupid to figure out that the if() inside MemSet evaluates to constant false in these calls? It seems hard to see how it would realize that the loop will write 16 bytes if it doesn't propagate the constant value forward. However ... on some other compilers, I've noticed that the compiler seems more likely to make "obvious" deductions of that sort if the variables in question are marked const. Does it help if you do - void *_vstart = (void *) (start); \ - int _val = (val); \ - Size _len = (len); \ + void * const _vstart = (void *) (start); \ + const int _val = (val); \ + const Size _len = (len); \ I don't think there's any strong reason not to just do s/MemSet/memset/ in these calls and nearby ones, but it would be good to understand just what's wrong here. And why it's only showing up in that file; seems nearly certain that we have similar coding elsewhere. regards, tom lane
-
Re: Windows warnings from VS 2017
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-09-21T00:16:28Z
On 09/20/2017 07:32 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 09/20/2017 06:13 PM, Michael Paquier wrote: >>> Those are around for some time, see here: >>> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com >>> But there has been no actual agreement about how to fix them.. >> Oh. Missed that. >> My solution was going to be slightly different. I was going to enclose >> the #ifdef'd code in a bare block and move the rte declaration inside >> that block. > Of the various solutions proposed in the previous thread, I think the > most salable alternative is probably ilmari's: get rid of the variable > and write the assert like > > Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY); > > That's a pretty minimal change and it doesn't add any cycles to the > non-Assert case. > > I can live with that. Will do. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Windows warnings from VS 2017
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-09-21T00:18:18Z
On 09/20/2017 07:54 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> It's also warning that it will copy 16 bytes to a 13 byte structure at >> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't >> seen any ill effects of this so far, but it seems to indicate that >> something is possibly amiss on this compiler with the MemSet macros. > That's weird. Is it too stupid to figure out that the if() inside > MemSet evaluates to constant false in these calls? It seems hard to > see how it would realize that the loop will write 16 bytes if it doesn't > propagate the constant value forward. > > However ... on some other compilers, I've noticed that the compiler seems > more likely to make "obvious" deductions of that sort if the variables in > question are marked const. Does it help if you do > > - void *_vstart = (void *) (start); \ > - int _val = (val); \ > - Size _len = (len); \ > + void * const _vstart = (void *) (start); \ > + const int _val = (val); \ > + const Size _len = (len); \ > > > I don't think there's any strong reason not to just do s/MemSet/memset/ > in these calls and nearby ones, but it would be good to understand just > what's wrong here. And why it's only showing up in that file; seems > nearly certain that we have similar coding elsewhere. > > I'll test it. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Windows warnings from VS 2017
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-09-21T02:26:42Z
On 09/20/2017 08:18 PM, Andrew Dunstan wrote: > > On 09/20/2017 07:54 PM, Tom Lane wrote: >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >>> It's also warning that it will copy 16 bytes to a 13 byte structure at >>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't >>> seen any ill effects of this so far, but it seems to indicate that >>> something is possibly amiss on this compiler with the MemSet macros. >> That's weird. Is it too stupid to figure out that the if() inside >> MemSet evaluates to constant false in these calls? It seems hard to >> see how it would realize that the loop will write 16 bytes if it doesn't >> propagate the constant value forward. >> >> However ... on some other compilers, I've noticed that the compiler seems >> more likely to make "obvious" deductions of that sort if the variables in >> question are marked const. Does it help if you do >> >> - void *_vstart = (void *) (start); \ >> - int _val = (val); \ >> - Size _len = (len); \ >> + void * const _vstart = (void *) (start); \ >> + const int _val = (val); \ >> + const Size _len = (len); \ >> >> >> I don't think there's any strong reason not to just do s/MemSet/memset/ >> in these calls and nearby ones, but it would be good to understand just >> what's wrong here. And why it's only showing up in that file; seems >> nearly certain that we have similar coding elsewhere. >> >> > > I'll test it. > Doesn't make a difference. I agree it would be good to understand what's going on. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Windows warnings from VS 2017
Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-09-21T06:53:43Z
On Thu, Sep 21, 2017 at 12:26 PM, Andrew Dunstan < andrew.dunstan@2ndquadrant.com> wrote: > > > On 09/20/2017 08:18 PM, Andrew Dunstan wrote: > > > > On 09/20/2017 07:54 PM, Tom Lane wrote: > >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > >>> It's also warning that it will copy 16 bytes to a 13 byte structure at > >>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I > haven't > >>> seen any ill effects of this so far, but it seems to indicate that > >>> something is possibly amiss on this compiler with the MemSet macros. > >> That's weird. Is it too stupid to figure out that the if() inside > >> MemSet evaluates to constant false in these calls? It seems hard to > >> see how it would realize that the loop will write 16 bytes if it doesn't > >> propagate the constant value forward. > >> > >> However ... on some other compilers, I've noticed that the compiler > seems > >> more likely to make "obvious" deductions of that sort if the variables > in > >> question are marked const. Does it help if you do > >> > >> - void *_vstart = (void *) (start); \ > >> - int _val = (val); \ > >> - Size _len = (len); \ > >> + void * const _vstart = (void *) (start); \ > >> + const int _val = (val); \ > >> + const Size _len = (len); \ > >> > >> > >> I don't think there's any strong reason not to just do s/MemSet/memset/ > >> in these calls and nearby ones, but it would be good to understand just > >> what's wrong here. And why it's only showing up in that file; seems > >> nearly certain that we have similar coding elsewhere. > >> > >> > > > > I'll test it. > > > > > Doesn't make a difference. I agree it would be good to understand what's > going on. These warnings are not produced when the build is run in DEBUG mode. Because of this I didn't see these warning when I was working with VS 2017. This may be an issue with VS 2017 compiler. Regards, Hari Babu Fujitsu Australia
-
Re: Windows warnings from VS 2017
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-09-21T12:11:35Z
On 09/21/2017 02:53 AM, Haribabu Kommi wrote: > > > On Thu, Sep 21, 2017 at 12:26 PM, Andrew Dunstan > <andrew.dunstan@2ndquadrant.com > <mailto:andrew.dunstan@2ndquadrant.com>> wrote: > > > > On 09/20/2017 08:18 PM, Andrew Dunstan wrote: > > > > On 09/20/2017 07:54 PM, Tom Lane wrote: > >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com > <mailto:andrew.dunstan@2ndquadrant.com>> writes: > >>> It's also warning that it will copy 16 bytes to a 13 byte > structure at > >>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. > I haven't > >>> seen any ill effects of this so far, but it seems to indicate that > >>> something is possibly amiss on this compiler with the MemSet > macros. > >> That's weird. Is it too stupid to figure out that the if() inside > >> MemSet evaluates to constant false in these calls? It seems > hard to > >> see how it would realize that the loop will write 16 bytes if > it doesn't > >> propagate the constant value forward. > >> > >> However ... on some other compilers, I've noticed that the > compiler seems > >> more likely to make "obvious" deductions of that sort if the > variables in > >> question are marked const. Does it help if you do > >> > >> - void *_vstart = (void *) (start); \ > >> - int _val = (val); \ > >> - Size _len = (len); \ > >> + void * const _vstart = (void *) (start); \ > >> + const int _val = (val); \ > >> + const Size _len = (len); \ > >> > >> > >> I don't think there's any strong reason not to just do > s/MemSet/memset/ > >> in these calls and nearby ones, but it would be good to > understand just > >> what's wrong here. And why it's only showing up in that file; > seems > >> nearly certain that we have similar coding elsewhere. > >> > >> > > > > I'll test it. > > > > > Doesn't make a difference. I agree it would be good to understand > what's > going on. > > > These warnings are not produced when the build is run in DEBUG mode. > Because of this I didn't see these warning when I was working with VS > 2017. > > This may be an issue with VS 2017 compiler. > Yeah, it seems like it, particularly when identical code in another function in the same file doesn't generate a complaint. The speed of memset is hardly going to be the dominating factor in a 'CREATE DATABASE' command, so we could certainly afford to change to plain memset calls here. I'll see if I can find someone to report this to :-) cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Windows warnings from VS 2017
Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-21T13:30:31Z
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > The speed of memset is hardly going to be the dominating factor in a > 'CREATE DATABASE' command, so we could certainly afford to change to > plain memset calls here. Another thought is that it may be time for our decennial debate about whether MemSet is worth the electrons it's printed on. I continue to think that any modern compiler+libc ought to do an equivalent or better optimization given just a plain memset(). If that seems to be true for recent MSVC, we could consider putting an #if into c.h to define MemSet as just memset for MSVC. Maybe later that could be extended to other compilers. regards, tom lane
-
Re: Windows warnings from VS 2017
Michael Paquier <michael.paquier@gmail.com> — 2017-09-22T01:36:14Z
On Thu, Sep 21, 2017 at 10:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Another thought is that it may be time for our decennial debate about > whether MemSet is worth the electrons it's printed on. I continue to > think that any modern compiler+libc ought to do an equivalent or better > optimization given just a plain memset(). If that seems to be true > for recent MSVC, we could consider putting an #if into c.h to define > MemSet as just memset for MSVC. Maybe later that could be extended > to other compilers. +1. Things change quickly in IT. -- Michael
-
Re: Windows warnings from VS 2017
Andres Freund <andres@anarazel.de> — 2017-09-22T01:41:08Z
On 2017-09-21 09:30:31 -0400, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > > The speed of memset is hardly going to be the dominating factor in a > > 'CREATE DATABASE' command, so we could certainly afford to change to > > plain memset calls here. > > Another thought is that it may be time for our decennial debate about > whether MemSet is worth the electrons it's printed on. I continue to > think that any modern compiler+libc ought to do an equivalent or better > optimization given just a plain memset(). If that seems to be true > for recent MSVC, we could consider putting an #if into c.h to define > MemSet as just memset for MSVC. Maybe later that could be extended > to other compilers. +many. glibc's memset is nearly an order of magnitude faster than MemSet on my machine for medium+ length copies, and still 3-4 four times ~100 bytes. And both gcc and clang optimize way the memcpy entirely when the length is a fixed short length. Greetings, Andres Freund
-
Re: Windows warnings from VS 2017
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-09-22T11:37:32Z
On 09/21/2017 09:41 PM, Andres Freund wrote: > On 2017-09-21 09:30:31 -0400, Tom Lane wrote: >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >>> The speed of memset is hardly going to be the dominating factor in a >>> 'CREATE DATABASE' command, so we could certainly afford to change to >>> plain memset calls here. >> Another thought is that it may be time for our decennial debate about >> whether MemSet is worth the electrons it's printed on. I continue to >> think that any modern compiler+libc ought to do an equivalent or better >> optimization given just a plain memset(). If that seems to be true >> for recent MSVC, we could consider putting an #if into c.h to define >> MemSet as just memset for MSVC. Maybe later that could be extended >> to other compilers. > +many. glibc's memset is nearly an order of magnitude faster than MemSet > on my machine for medium+ length copies, and still 3-4 four times ~100 > bytes. And both gcc and clang optimize way the memcpy entirely when the > length is a fixed short length. Perhaps we need some sort of small benchmark program that we can run on a representative set of platforms? I presume if you can make that assertion you already have something along those lines? cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Windows warnings from VS 2017
Andres Freund <andres@anarazel.de> — 2017-10-11T21:28:16Z
> On 09/21/2017 09:41 PM, Andres Freund wrote: > > On 2017-09-21 09:30:31 -0400, Tom Lane wrote: > >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > >>> The speed of memset is hardly going to be the dominating factor in a > >>> 'CREATE DATABASE' command, so we could certainly afford to change to > >>> plain memset calls here. > >> Another thought is that it may be time for our decennial debate about > >> whether MemSet is worth the electrons it's printed on. I continue to > >> think that any modern compiler+libc ought to do an equivalent or better > >> optimization given just a plain memset(). If that seems to be true > >> for recent MSVC, we could consider putting an #if into c.h to define > >> MemSet as just memset for MSVC. Maybe later that could be extended > >> to other compilers. > > +many. glibc's memset is nearly an order of magnitude faster than MemSet > > on my machine for medium+ length copies, and still 3-4 four times ~100 > > bytes. And both gcc and clang optimize way the memcpy entirely when the > > length is a fixed short length. > Perhaps we need some sort of small benchmark program that we can run on > a representative set of platforms? I personally don't think that's needed here, given how incredibly widely used memset is in our codebase. > I presume if you can make that assertion you already have something > along those lines? Not really. I just replaced memset with MemSetAligned in a bunch of places in the code and looked at profiles. Greetings, Andres Freund
-
Re: Windows warnings from VS 2017
Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-11T22:20:24Z
Andres Freund <andres@anarazel.de> writes: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> I presume if you can make that assertion you already have something >> along those lines? > Not really. I just replaced memset with MemSetAligned in a bunch of > places in the code and looked at profiles. Well, it's not that much work to try it and see. I compared results of this simplistic test case: pgbench -S -c 1 -T 60 bench (using a scale-factor-10 pgbench database) on current HEAD and HEAD with the attached patch, which just lobotomizes all the MemSet macros into memset(). Median of 3 runs is 9698 tps for HEAD and 9675 tps with the patch; the range is ~100 tps though, making this difference well within the noise level. I did this using RHEL6's gcc (Red Hat 4.4.7-18), which is pretty far from bleeding-edge so I think it's okay as a baseline for what optimizations we can expect to find used in the field. So I can't sustain Andres' assertion that memset is actually faster for the cases we care about, but it certainly doesn't seem any slower either. It would be interesting to check compromise patches, such as keeping only the MemSetLoop case. BTW, I got a couple of warnings with this patch: pmsignal.c: In function 'PMSignalShmemInit': pmsignal.c:104: warning: passing argument 1 of 'memset' discards qualifiers from pointer target type /usr/include/string.h:65: note: expected 'void *' but argument is of type 'volatile struct PMSignalData *' procsignal.c: In function 'ProcSignalInit': procsignal.c:119: warning: passing argument 1 of 'memset' discards qualifiers from pointer target type /usr/include/string.h:65: note: expected 'void *' but argument is of type 'volatile sig_atomic_t *' Those seem like maybe something to look into. MemSet is evidently casting away pointer properties that it perhaps shouldn't. regards, tom lane
-
Re: Windows warnings from VS 2017
Andres Freund <andres@anarazel.de> — 2017-10-17T20:17:10Z
On 2017-10-11 18:20:24 -0400, Tom Lane wrote: > Well, it's not that much work to try it and see. I compared results > of this simplistic test case: > pgbench -S -c 1 -T 60 bench > (using a scale-factor-10 pgbench database) on current HEAD and HEAD > with the attached patch, which just lobotomizes all the MemSet > macros into memset(). Median of 3 runs is 9698 tps for HEAD and > 9675 tps with the patch; the range is ~100 tps though, making > this difference well within the noise level. > > I did this using RHEL6's gcc (Red Hat 4.4.7-18), which is pretty > far from bleeding-edge so I think it's okay as a baseline for > what optimizations we can expect to find used in the field. > > So I can't sustain Andres' assertion that memset is actually faster > for the cases we care about, but it certainly doesn't seem any > slower either. It would be interesting to check compromise > patches, such as keeping only the MemSetLoop case. Well, that'd be because that workload doesn't exercise either version of memset to a meaningful degree, right? > +#define MemSetAligned(start, val, len) memset(start, val, len) If we actually care about this optimization, which seems to solely serve palloc0fast, we could tell the compiler about the guaranteed alignment etc. I'm doubtful it's worth making this work for palloc0fast alone, but I think it might be worthwhile to make palloc0 an inline function like #ifdef YES_I_AM_A_GOOD_COMPILER /* not needed here, but probably good for plenty other places */ #define pg_attribute_assume_aligned(align) __attribute__((assume_aligned(align))) #define pg_assume_aligned(ptr, align) __builtin_assume_aligned(ptr, align) #else ... #end extern void *palloc(Size size) pg_attribute_assume_aligned(MAXALIGN); static inline void * palloc0(Size size) { void *mem = palloc(size); memset(mem, 0, size); return mem; } that should allow the compiler to always optimize the memset based on the alignment - at least x86 gcc does so - and on the size if a built-time constant (nearly every compiler does so). I assume we probably should do this dance not just for palloc, but for the other allocation functions too. Greetings, Andres Freund