Re: [HACKERS] Windows warnings from VS 2017

[Andres Freund]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Tom Lane]

Andres Freund  writes:
> Andrew Dunstan  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

diff --git a/src/include/c.h b/src/include/c.h
index b6a9697..f7ea3a5 100644
*** a/src/include/c.h
--- b/src/include/c.h
*** typedef NameData *Name;
*** 843,874 
   *	memset() functions.  More research needs to be done, perhaps with
   *	MEMSET_LOOP_LIMIT tests in configure.
   */
! #define MemSet(start, val, len) \
! 	do \
! 	{ \
! 		/* must be void* because we don't know if it is integer aligned yet */ \
! 		void   *_vstart = (void *) (start); \
! 		int		_val = (val); \
! 		Size	_len = (len); \
! \
! 		if uintptr_t) _vstart) & LONG_ALIGN_MASK) == 0 && \
! 			(_len & LONG_ALIGN_MASK) == 0 && \
! 			_val == 0 && \
! 			_len <= MEMSET_LOOP_LIMIT && \
! 			/* \
! 			 *	If MEMSET_LOOP_LIMIT == 0, optimizer should find \
! 			 *	the whole "if" false at compile time. \
! 			 */ \
! 			MEMSET_LOOP_LIMIT != 0) \
! 		{ \
! 			long *_start = (long *) _vstart; \
! 			long *_stop = (long *) ((char *) _start + _len); \
! 			while (_start < _stop) \
! *_start++ = 0; \
! 		} \
! 		else \
! 			memset(_vstart, _val, _len); \
! 	} while (0)
  
  /*
   * MemSetAligned is the same as MemSet except it omits the test to see if
--- 843,849 
   *	memset() functions.  More research needs to be done, perhaps with
   *	MEMSET_LOOP_LIMIT tests in configure.
   */
! #define MemSet(start, val, len) memset(start, val, len)
  
  /*
   * MemSetAligned is the same as MemSet except it omits the test to see if
*** typedef NameData *Name;
*** 876,901 
   * that the pointer is suitably aligned (typically, because he just got it
   * from palloc(), which always delivers a max-aligned pointer).
   */
! #define MemSetAligned(start, val, len) \
! 	do \
! 	{ \
! 		long   *_start = (long *) (start); \
! 		int		_val = (val); \
! 		Size	_len = (len); \
! \
! 		if ((_len & LONG_ALIGN_MASK) == 0 && \
! 			_val == 0 && \
! 			_len <= MEMSET_LOOP_LIMIT && \
! 			MEMSET_LOOP_LIMIT != 0) \
! 		{ \
! 			long *_stop = (long *) ((char *) _start + _len); \
! 			while (_start < _stop) \
! *_start++ = 0; \
! 		} \
! 		else \
! 			memset(_start, _val, _len); \
! 	} while (0)
! 
  
  /*
   * MemSetTest/MemSetLoop are a variant version that allow all the tests in
--- 851,857 
   * that the pointer is suitably aligned (typically, because he just got it
   * from palloc(), which always delivers a max-aligned pointer).
   */
! #define MemSetAligned(start, val, len) memset(start, val, len)
  
  /*
   * MemSetTest/MemSetLoop are a variant version that allow all the tests in
*** typedef NameData *Name;
*** 911,925 
  	MEMSET_LOOP_LIMIT != 0 && \
  	(val) == 0 )
  
! #define MemSetLoop(start, val, len) \
! 	do \
! 	{ \
! 		long * _start = (long *) (start); \
! 		long * _stop = (long *) ((char *) _start + (Size) (len)); \
! 	\
! 		while (_start < _stop) \
! 			*_start++ = 0; \
! 	} while (0)
  
  
  /*
--- 867,873 
  	MEMSET_LOOP_LIMIT != 0 && \
  	(val) == 0 )
  
! #define MemSetLoop(start, val, len) memset(start, val, len)
  
  
  /*

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your 

Re: [HACKERS] Windows warnings from VS 2017

[Andres Freund]

> On 09/21/2017 09:41 PM, Andres Freund wrote:
> > On 2017-09-21 09:30:31 -0400, Tom Lane wrote:
> >> Andrew Dunstan  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/21/2017 09:41 PM, Andres Freund wrote:
> On 2017-09-21 09:30:31 -0400, Tom Lane wrote:
>> Andrew Dunstan  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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Andres Freund]

On 2017-09-21 09:30:31 -0400, Tom Lane wrote:
> Andrew Dunstan  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Michael Paquier]

On Thu, Sep 21, 2017 at 10:30 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Tom Lane]

Andrew Dunstan  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/21/2017 02:53 AM, Haribabu Kommi wrote:
>
>
> On Thu, Sep 21, 2017 at 12:26 PM, Andrew Dunstan
>  > wrote:
>
>
>
> On 09/20/2017 08:18 PM, Andrew Dunstan wrote:
> >
> > On 09/20/2017 07:54 PM, Tom Lane wrote:
> >> Andrew Dunstan  > 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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Haribabu Kommi]

On Thu, Sep 21, 2017 at 12:26 PM, Andrew Dunstan <
andrew.duns...@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  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: [HACKERS] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/20/2017 08:18 PM, Andrew Dunstan wrote:
>
> On 09/20/2017 07:54 PM, Tom Lane wrote:
>> Andrew Dunstan  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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/20/2017 07:54 PM, Tom Lane wrote:
> Andrew Dunstan  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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/20/2017 07:32 PM, Tom Lane wrote:
> Andrew Dunstan  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+4ufqjggrv8osqbuzzxg...@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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Tom Lane]

Andrew Dunstan  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Tom Lane]

Andrew Dunstan  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+4ufqjggrv8osqbuzzxg...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Michael Paquier]

On Thu, Sep 21, 2017 at 7:49 AM, Andrew Dunstan
 wrote:
> On 09/20/2017 06:13 PM, Michael Paquier wrote:
>> On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan
>>  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+4ufqjggrv8osqbuzzxg...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Andrew Dunstan]

On 09/20/2017 06:13 PM, Michael Paquier wrote:
> On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan
>  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+4ufqjggrv8osqbuzzxg...@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 Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Windows warnings from VS 2017

[Michael Paquier]

On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan
 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+4ufqjggrv8osqbuzzxg...@mail.gmail.com
But there has been no actual agreement about how to fix them..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers