Re: [HACKERS] Compiler warning in costsize.c

[David Rowley]

On 11 April 2017 at 12:53, Michael Paquier  wrote:
> On Tue, Apr 11, 2017 at 4:02 AM, Robert Haas  wrote:
>> On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane  wrote:
>>> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
 Why bother with the 'rte' variable at all if it's only used for the
 Assert()ing the rtekind?
>>>
>>> That was proposed a few messages back.  I don't like it because it makes
>>> these functions look different from the other scan-cost-estimation
>>> functions, and we'd just have to undo the "optimization" if they ever
>>> grow a need to reference the rte for another purpose.
>>
>> I think that's sort of silly, though.  It's a trivial difference,
>> neither likely to confuse anyone nor difficult to undo.
>
> +1. I would just do that and call it a day. There is no point to do a
> mandatory list lookup as that's just for an assertion, and fixing this
> warning does not seem worth the addition of fancier facilities. If the
> function declarations were doubly-nested in the code, I would
> personally consider the use of a variable, but not here.

Any more thoughts on what is acceptable for fixing this? beta1 is
looming and it seems a bit messy to be shipping that with these
warnings, however harmless they are.


-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] compiler warning with VS 2017

[Tom Lane]

Petr Jelinek  writes:
> On 05/05/17 16:56, Tom Lane wrote:
>> So the comment should be something like "if the column is unchanged,
>> we should not attempt to access its value beyond this point.  To
>> help catch any such attempts, set the string to NULL" ?

> Yes that sounds about right. We don't get any data for unchanged TOAST
> columns (that's limitation of logical decoding) so we better not touch them.

OK.  I just made it say "we don't get the value of unchanged columns".

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] compiler warning with VS 2017

[Petr Jelinek]

On 05/05/17 16:56, Tom Lane wrote:
> Petr Jelinek  writes:
>> On 05/05/17 06:50, Tom Lane wrote:
>>> Actually, looking around a bit there, it's not even clear why
>>> we should be booby-trapping the value of an unchanged column in
>>> the first place.  So I'd say that not only is the code dubious
>>> but the comment is inadequate too.
> 
>> Hmm, as far as I can recollect this is just leftover debugging code that
>> was intended to help ensure that we are checking the "changed"
>> everywhere we are supposed to (since I changed handling of these
>> structured quite a bit during development). Should be changed to NULL,
>> that's what we usually do in this type of situation.
> 
> So the comment should be something like "if the column is unchanged,
> we should not attempt to access its value beyond this point.  To
> help catch any such attempts, set the string to NULL" ?
> 

Yes that sounds about right. We don't get any data for unchanged TOAST
columns (that's limitation of logical decoding) so we better not touch them.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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] compiler warning with VS 2017

[Tom Lane]

Petr Jelinek  writes:
> On 05/05/17 06:50, Tom Lane wrote:
>> Actually, looking around a bit there, it's not even clear why
>> we should be booby-trapping the value of an unchanged column in
>> the first place.  So I'd say that not only is the code dubious
>> but the comment is inadequate too.

> Hmm, as far as I can recollect this is just leftover debugging code that
> was intended to help ensure that we are checking the "changed"
> everywhere we are supposed to (since I changed handling of these
> structured quite a bit during development). Should be changed to NULL,
> that's what we usually do in this type of situation.

So the comment should be something like "if the column is unchanged,
we should not attempt to access its value beyond this point.  To
help catch any such attempts, set the string to NULL" ?

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] compiler warning with VS 2017

[Petr Jelinek]

On 05/05/17 06:50, Tom Lane wrote:
> Haribabu Kommi  writes:
>> I am getting a compiler warning when I build the latest HEAD PostgreSQL with
>> visual studio 2017.
>> The code at the line is,
>> tuple->values[i] = (char *) (Size)0xdeadbeef; /* make bad usage more obvious 
>> */
> 
> Yeah, you're not the first to complain about this.  To my mind that
> coding is not pretty, not cute, and not portable: there's not even
> a good reason to believe that dereferencing the pointer would result
> in a crash.  Perhaps the author can explain to us why this is better
> than just assigning NULL.
> 
> Actually, looking around a bit there, it's not even clear why
> we should be booby-trapping the value of an unchanged column in
> the first place.  So I'd say that not only is the code dubious
> but the comment is inadequate too.

Hmm, as far as I can recollect this is just leftover debugging code that
was intended to help ensure that we are checking the "changed"
everywhere we are supposed to (since I changed handling of these
structured quite a bit during development). Should be changed to NULL,
that's what we usually do in this type of situation.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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] compiler warning with VS 2017

[Tom Lane]

Haribabu Kommi  writes:
> I am getting a compiler warning when I build the latest HEAD PostgreSQL with
> visual studio 2017.
> The code at the line is,
> tuple->values[i] = (char *) (Size)0xdeadbeef; /* make bad usage more obvious 
> */

Yeah, you're not the first to complain about this.  To my mind that
coding is not pretty, not cute, and not portable: there's not even
a good reason to believe that dereferencing the pointer would result
in a crash.  Perhaps the author can explain to us why this is better
than just assigning NULL.

Actually, looking around a bit there, it's not even clear why
we should be booby-trapping the value of an unchanged column in
the first place.  So I'd say that not only is the code dubious
but the comment is inadequate too.

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] Compiler warning in costsize.c

[Michael Paquier]

On Tue, Apr 11, 2017 at 4:02 AM, Robert Haas  wrote:
> On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane  wrote:
>> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>>> Why bother with the 'rte' variable at all if it's only used for the
>>> Assert()ing the rtekind?
>>
>> That was proposed a few messages back.  I don't like it because it makes
>> these functions look different from the other scan-cost-estimation
>> functions, and we'd just have to undo the "optimization" if they ever
>> grow a need to reference the rte for another purpose.
>
> I think that's sort of silly, though.  It's a trivial difference,
> neither likely to confuse anyone nor difficult to undo.

+1. I would just do that and call it a day. There is no point to do a
mandatory list lookup as that's just for an assertion, and fixing this
warning does not seem worth the addition of fancier facilities. If the
function declarations were doubly-nested in the code, I would
personally consider the use of a variable, but not here.
-- 
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] Compiler warning in costsize.c

[Robert Haas]

On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane  wrote:
> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> Why bother with the 'rte' variable at all if it's only used for the
>> Assert()ing the rtekind?
>
> That was proposed a few messages back.  I don't like it because it makes
> these functions look different from the other scan-cost-estimation
> functions, and we'd just have to undo the "optimization" if they ever
> grow a need to reference the rte for another purpose.

I think that's sort of silly, though.  It's a trivial difference,
neither likely to confuse anyone nor difficult to undo.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Compiler warning in costsize.c

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Why bother with the 'rte' variable at all if it's only used for the
> Assert()ing the rtekind?

That was proposed a few messages back.  I don't like it because it makes
these functions look different from the other scan-cost-estimation
functions, and we'd just have to undo the "optimization" if they ever
grow a need to reference the rte for another purpose.

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] Compiler warning in costsize.c

[Tom Lane]

Robert Haas  writes:
> On Mon, Apr 10, 2017 at 8:30 AM, Michael Paquier
>  wrote:
>> On Mon, Apr 10, 2017 at 9:05 PM, Tom Lane  wrote:
>>> I wonder if we shouldn't just do
>>> ...
>>> and eat the "useless" calculation of rte.

> -1 from me.  I'm not a big fan of useless calculation just because it
> happens to be needed in an Assert-enabled build.

Well, those planner_rt_fetch() calls are going to reduce to a simple
array lookup, so it seems rather extreme to insist on contorting the
code just to avoid that.  It's not like these functions are trivially
cheap otherwise.

In fact, I kind of wonder why we're using planner_rt_fetch() at all in
costsize.c, rather than "root->simple_rte_array[rel->relid]".  Maybe at
one time these functions were invokable before reaching query_planner(),
but we don't do that anymore.  (Just to be sure, I stuck
"Assert(root->simple_rte_array)" into each costsize.c function that uses
planner_rt_fetch, and it still passes check-world.)

So now my proposal is

/* Should only be applied to base relations that are subqueries */
Assert(rel->relid > 0);
-#ifdef USE_ASSERT_CHECKING
-   rte = planner_rt_fetch(rel->relid, root);
+   rte = root->simple_rte_array[rel->relid];
Assert(rte->rtekind == RTE_SUBQUERY);
-#endif

and make the rest of costsize.c look like that too.

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] Compiler warning in costsize.c

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> I wonder if we shouldn't just do
>
>   RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
>   ListCell   *lc;
>  
>   /* Should only be applied to base relations that are subqueries */
>   Assert(rel->relid > 0);
> -#ifdef USE_ASSERT_CHECKING
>   rte = planner_rt_fetch(rel->relid, root);
>   Assert(rte->rtekind == RTE_SUBQUERY);
> -#endif
>
> and eat the "useless" calculation of rte.

Why bother with the 'rte' variable at all if it's only used for the
Assert()ing the rtekind?

Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY);

- ilmari

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


-- 
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] Compiler warning in costsize.c

[Robert Haas]

On Mon, Apr 10, 2017 at 8:30 AM, Michael Paquier
 wrote:
> On Mon, Apr 10, 2017 at 9:05 PM, Tom Lane  wrote:
>> I wonder if we shouldn't just do
>>
>> RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
>> ListCell   *lc;
>>
>> /* Should only be applied to base relations that are subqueries */
>> Assert(rel->relid > 0);
>> -#ifdef USE_ASSERT_CHECKING
>> rte = planner_rt_fetch(rel->relid, root);
>> Assert(rte->rtekind == RTE_SUBQUERY);
>> -#endif
>>
>> and eat the "useless" calculation of rte.
>
> That works as well. Now this code really has been written so as we
> don't want to do this useless computation for non-Assert builds,
> that's why I did not suggest it. But as it does just a list_nth call,
> that's not really costly... And other code paths dealing with the cost
> do it as well.

-1 from me.  I'm not a big fan of useless calculation just because it
happens to be needed in an Assert-enabled build.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Compiler warning in costsize.c

[Michael Paquier]

On Mon, Apr 10, 2017 at 9:05 PM, Tom Lane  wrote:
> I wonder if we shouldn't just do
>
> RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
> ListCell   *lc;
>
> /* Should only be applied to base relations that are subqueries */
> Assert(rel->relid > 0);
> -#ifdef USE_ASSERT_CHECKING
> rte = planner_rt_fetch(rel->relid, root);
> Assert(rte->rtekind == RTE_SUBQUERY);
> -#endif
>
> and eat the "useless" calculation of rte.

That works as well. Now this code really has been written so as we
don't want to do this useless computation for non-Assert builds,
that's why I did not suggest it. But as it does just a list_nth call,
that's not really costly... And other code paths dealing with the cost
do it as well.
-- 
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] Compiler warning in costsize.c

[Tom Lane]

David Rowley  writes:
> On 8 April 2017 at 04:42, Tom Lane  wrote:
>> BTW, is it really true that only these two places produce such warnings
>> on MSVC?  I see about three dozen uses of PG_USED_FOR_ASSERTS_ONLY in our
>> tree, and I'd have expected all of those places to be causing warnings on
>> a compiler that doesn't have a way to understand that annotation.

> Seems that MSVC is happy once the variable is assigned, and does not
> bother checking if the variable is used after being assigned, whereas,
> some other compilers might see the variable as uselessly assigned.

> At the moment there are no other warnings from MSVC since all the
> other places the variable gets assigned a value in some code path.

I wonder if we shouldn't just do

RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
ListCell   *lc;
 
/* Should only be applied to base relations that are subqueries */
Assert(rel->relid > 0);
-#ifdef USE_ASSERT_CHECKING
rte = planner_rt_fetch(rel->relid, root);
Assert(rte->rtekind == RTE_SUBQUERY);
-#endif

and eat the "useless" calculation of rte.

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] Compiler warning in costsize.c

[David Rowley]

On 8 April 2017 at 04:42, Tom Lane  wrote:
> I'd be happier with something along the line of
>
> RangeTblEntry *rte;
> ListCell   *lc;
>
> /* Should only be applied to base relations that are subqueries */
> Assert(rel->relid > 0);
> rte = planner_rt_fetch(rel->relid, root);
> #ifdef USE_ASSERT_CHECKING
> Assert(rte->rtekind == RTE_SUBQUERY);
> #else
> (void) rte;  /* silence unreferenced-variable complaints */
> #endif

the (void) rte; would not be required to silence MSVC here. Of course,
PG_USED_FOR_ASSERTS_ONLY would be required to stop some smarter
compiler from complaining.

> assuming that that actually does silence the warning on MSVC.
>
> BTW, is it really true that only these two places produce such warnings
> on MSVC?  I see about three dozen uses of PG_USED_FOR_ASSERTS_ONLY in our
> tree, and I'd have expected all of those places to be causing warnings on
> a compiler that doesn't have a way to understand that annotation.

Seems that MSVC is happy once the variable is assigned, and does not
bother checking if the variable is used after being assigned, whereas,
some other compilers might see the variable as uselessly assigned.

At the moment there are no other warnings from MSVC since all the
other places the variable gets assigned a value in some code path.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] Compiler warning in costsize.c

[Tom Lane]

Michael Paquier  writes:
> Bah. This actually fixes nothing. Attached is a different patch that
> really addresses the problem, by removing the variable because we
> don't want planner_rt_fetch() to run for non-Assert builds.

I don't really like any of these fixes, because they take the code
further away from the structure used by all the other similar functions
in costsize.c, and they will be hard to undo whenever these functions
grow a reason to look at the RTE normally (outside asserts).

I'd be happier with something along the line of

RangeTblEntry *rte;
ListCell   *lc;

/* Should only be applied to base relations that are subqueries */
Assert(rel->relid > 0);
rte = planner_rt_fetch(rel->relid, root);
#ifdef USE_ASSERT_CHECKING
Assert(rte->rtekind == RTE_SUBQUERY);
#else
(void) rte;  /* silence unreferenced-variable complaints */
#endif

assuming that that actually does silence the warning on MSVC.

BTW, is it really true that only these two places produce such warnings
on MSVC?  I see about three dozen uses of PG_USED_FOR_ASSERTS_ONLY in our
tree, and I'd have expected all of those places to be causing warnings on
a compiler that doesn't have a way to understand that annotation.

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] Compiler warning in costsize.c

[Michael Paquier]

On Fri, Apr 7, 2017 at 12:38 AM, Michael Paquier
 wrote:
> On Tue, Apr 4, 2017 at 9:42 PM, Michael Paquier
>  wrote:
>> On Wed, Apr 5, 2017 at 2:54 AM, Tom Lane  wrote:
>>> (I'm personally not that much in love with PG_USED_FOR_ASSERTS_ONLY,
>>> because it tends to confuse pgindent.)
>>
>> I would be incline to just do that, any other solution I can think of
>> is uglier than that.
>
> Actually, no. Looking at this issue again the warning is triggered
> because the Assert() clause is present in USE_ASSERT_CHECKING. So
> instead of removing PG_USED_FOR_ASSERTS_ONLY, here is a more simple
> patch that fixes the problem. No need to put the variable *rte within
> ifdefs as well.

Bah. This actually fixes nothing. Attached is a different patch that
really addresses the problem, by removing the variable because we
don't want planner_rt_fetch() to run for non-Assert builds.
-- 
Michael


costsize-warning-fix-3.patch
Description: Binary data

-- 
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] Compiler warning in costsize.c

[Michael Paquier]

On Tue, Apr 4, 2017 at 9:42 PM, Michael Paquier
 wrote:
> On Wed, Apr 5, 2017 at 2:54 AM, Tom Lane  wrote:
>> (I'm personally not that much in love with PG_USED_FOR_ASSERTS_ONLY,
>> because it tends to confuse pgindent.)
>
> I would be incline to just do that, any other solution I can think of
> is uglier than that.

Actually, no. Looking at this issue again the warning is triggered
because the Assert() clause is present in USE_ASSERT_CHECKING. So
instead of removing PG_USED_FOR_ASSERTS_ONLY, here is a more simple
patch that fixes the problem. No need to put the variable *rte within
ifdefs as well.
-- 
Michael


costsize-warning-fix-2.patch
Description: Binary data

-- 
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] Compiler warning in costsize.c

[Michael Paquier]

On Wed, Apr 5, 2017 at 2:54 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> In builds where USE_ASSERT_CHECKING is not enabled, costsize.c can
>> generate warnings. Here is for example with MSVC:
>>   src/backend/optimizer/path/costsize.c(4520): warning C4101: 'rte' : 
>> unreferenced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]
>>   src/backend/optimizer/path/costsize.c(4640): warning C4101: 'rte' : 
>> unreferenced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]
>
>> a9c074ba7 has done an effort, but a bit more is needed as the attached.
>
> That doesn't look right at all:
>
> +#ifdef USE_ASSERT_CHECKING
> RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
> +#endif
>
> The entire point of the PG_USED_FOR_ASSERTS_ONLY annotation is to suppress
> this type of warning without a need for an explicit #ifdef like that one.
>
> We should either fix PG_USED_FOR_ASSERTS_ONLY to work on MSVC as well,
> or decide that we don't care about suppressing such warnings on MSVC,
> or give up on PG_USED_FOR_ASSERTS_ONLY and remove it everywhere in
> favor of plain #ifdefs.

Visual does not have any equivalent of __attribute__((unused))... And
visual does not have an equivalent after looking around. A trick that
I can think of would be to change PG_USED_FOR_ASSERTS_ONLY to be
roughly a macro like that:

#if defined(__GNUC__)
#define PG_ASSERT_ONLY(x) ASSERT_ONLY_ ## x __attribute__((unused))
#else
#define PG_ASSERT_ONLY(x) ((void) x)
#endif

But that's ugly...

> (I'm personally not that much in love with PG_USED_FOR_ASSERTS_ONLY,
> because it tends to confuse pgindent.)

I would be incline to just do that, any other solution I can think of
is uglier than that.
-- 
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] Compiler warning in costsize.c

[Tom Lane]

Michael Paquier  writes:
> In builds where USE_ASSERT_CHECKING is not enabled, costsize.c can
> generate warnings. Here is for example with MSVC:
>   src/backend/optimizer/path/costsize.c(4520): warning C4101: 'rte' : 
> unreferenced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]
>   src/backend/optimizer/path/costsize.c(4640): warning C4101: 'rte' : 
> unreferenced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]

> a9c074ba7 has done an effort, but a bit more is needed as the attached.

That doesn't look right at all:

+#ifdef USE_ASSERT_CHECKING
RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
+#endif

The entire point of the PG_USED_FOR_ASSERTS_ONLY annotation is to suppress
this type of warning without a need for an explicit #ifdef like that one.

We should either fix PG_USED_FOR_ASSERTS_ONLY to work on MSVC as well,
or decide that we don't care about suppressing such warnings on MSVC,
or give up on PG_USED_FOR_ASSERTS_ONLY and remove it everywhere in
favor of plain #ifdefs.

(I'm personally not that much in love with PG_USED_FOR_ASSERTS_ONLY,
because it tends to confuse pgindent.)

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] Compiler warning in costsize.c

[Michael Paquier]

On Tue, Apr 4, 2017 at 7:03 PM, David Rowley
 wrote:
> On 4 April 2017 at 16:22, Michael Paquier  wrote:
>> Hi all,
>>
>> In builds where USE_ASSERT_CHECKING is not enabled, costsize.c can
>> generate warnings. Here is for example with MSVC:
>>   src/backend/optimizer/path/costsize.c(4520): warning C4101: 'rte' : 
>> unreferen
>> ced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]
>>   src/backend/optimizer/path/costsize.c(4640): warning C4101: 'rte' : 
>> unreferen
>> ced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]
>>
>> a9c074ba7 has done an effort, but a bit more is needed as the attached.
>
> Thanks for writing the patch.
>
> I wondering if it would be worth simplifying it a little to get rid of
> the double #ifdefs, as per attached.

Yes, that would be fine as well.
-- 
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] Compiler warning in costsize.c

[David Rowley]

On 4 April 2017 at 16:22, Michael Paquier  wrote:
> Hi all,
>
> In builds where USE_ASSERT_CHECKING is not enabled, costsize.c can
> generate warnings. Here is for example with MSVC:
>   src/backend/optimizer/path/costsize.c(4520): warning C4101: 'rte' : 
> unreferen
> ced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]
>   src/backend/optimizer/path/costsize.c(4640): warning C4101: 'rte' : 
> unreferen
> ced local variable [C:\Users\ioltas\git\postgres\postgres.vcxproj]
>
> a9c074ba7 has done an effort, but a bit more is needed as the attached.

Thanks for writing the patch.

I wondering if it would be worth simplifying it a little to get rid of
the double #ifdefs, as per attached.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


costsize-fix-warning_drowley.patch
Description: Binary data

-- 
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] compiler warning in set_tablefunc_size_estimates

[Robert Haas]

On Thu, Mar 9, 2017 at 4:39 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I tried a non-cassert compile on a machine that has a pickier compiler
>> than my laptop, and got:
>
>> costsize.c: In function ‘set_tablefunc_size_estimates’:
>> costsize.c:4574:17: error: variable ‘rte’ set but not used
>> [-Werror=unused-but-set-variable]
>
>> That appears to be a legitimate gripe.  Perhaps:
>
> I think PG_USED_FOR_ASSERTS_ONLY would be a better solution.  It's
> only happenstance that the function currently uses the RTE just
> for this; if it grows another use, your approach would be harder
> to clean up.

Yeah, we might have to revert the entire -4/+1 line patch.

Actually, the thing I don't like about that is that that then we're
still emitting code for the planner_rt_fetch.  That probably doesn't
cost much, but why do it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] compiler warning in set_tablefunc_size_estimates

[Tom Lane]

Robert Haas  writes:
> I tried a non-cassert compile on a machine that has a pickier compiler
> than my laptop, and got:

> costsize.c: In function ‘set_tablefunc_size_estimates’:
> costsize.c:4574:17: error: variable ‘rte’ set but not used
> [-Werror=unused-but-set-variable]

> That appears to be a legitimate gripe.  Perhaps:

I think PG_USED_FOR_ASSERTS_ONLY would be a better solution.  It's
only happenstance that the function currently uses the RTE just
for this; if it grows another use, your approach would be harder
to clean up.

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] Compiler warning

[Magnus Hagander]

On Dec 24, 2016 01:21, "Bruce Momjian"  wrote:

I am seeing this compiler warning in the 9.4 branch:

 9.4:  basebackup.c:1284:6: warning: variable 'wait_result' set but not
used [-Wunused-but-set-variable]

This is on Debian Jessie with gcc version 4.9.2.  It is from this commit:

commit f6508827afe76b2c3735a9ce073620e708d60c79



Hi!

This was already reported by Dean back on the thread on - committers,
including one question still to be investigated. I plan to get back to it
when I get back from Christmas holidays.

/Magnus

Re: [HACKERS] compiler warning read_objtype_from_string()

[Alvaro Herrera]

Tom Lane wrote:

> I do not think you should assume that the compiler is smart enough to
> deduce that, nor that all compilers even know ereport(ERROR) doesn't
> return.  Personally I don't see the point of the "type" variable at
> all, anyway.  I would have written this as
> 
> [code]

Makes sense.  I will patch it that way.

-- 
Álvaro Herrerahttps://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] compiler warning read_objtype_from_string()

[Tom Lane]

Alvaro Herrera  writes:
> Peter Eisentraut wrote:
>> I'm getting the following compiler warning (using nondefault
>> optimization options):
>> objectaddress.c: In function 'read_objtype_from_string':
>> objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
>> function [-Werror=maybe-uninitialized]
>> return type;

> Umm.  I think it can only be uninitialized if we fall out of the end of
> the array, in which case we're supposed to throw the ERROR and never
> return.  Is that not working?

I do not think you should assume that the compiler is smart enough to
deduce that, nor that all compilers even know ereport(ERROR) doesn't
return.  Personally I don't see the point of the "type" variable at
all, anyway.  I would have written this as

inti;

for (i = 0; i < lengthof(ObjectTypeMap); i++)
{
if (strcmp(ObjectTypeMap[i].tm_name, objtype) == 0)
return ObjectTypeMap[i].tm_type;
}
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 errmsg("unrecognized object type \"%s\"", objtype)));
return -1;/* keep compiler quiet */

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] compiler warning read_objtype_from_string()

[Alvaro Herrera]

Peter Eisentraut wrote:
> I'm getting the following compiler warning (using nondefault
> optimization options):
> 
> objectaddress.c: In function 'read_objtype_from_string':
> objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
>   return type;

Umm.  I think it can only be uninitialized if we fall out of the end of
the array, in which case we're supposed to throw the ERROR and never
return.  Is that not working?

> The comment for the function says
> 
>  * Return ObjectType for the given object type as given by
>  * getObjectTypeDescription; if no valid ObjectType code exists, but it's a
>  * possible output type from getObjectTypeDescription, return -1.
> 
> But the claim that it can return -1 does not seem supported by the code.

Actually, it is -- but the -1 value comes from the ObjectType array.
Perhaps the comment should state that explicitely.

-- 
Álvaro Herrerahttps://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] Compiler warning in pg_am changes

[Tom Lane]

David Rowley  writes:
> I've attached a small patch to fix new compiler warning which is new as of
> 65c5fcd3

Pushed, thanks.

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] Compiler warning

[Tom Lane]

Peter Geoghegan  writes:
> Commit 7e2a18a9 must have caused this compiler warning which I now see
> on the master branch with my standard release build settings:

[ scratches head... ]  Dunno why my compiler didn't complain about that.
Will fix it in a bit.

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] Compiler warning about overflow in xlog.c

[Tom Lane]

Petr Jelinek p...@2ndquadrant.com writes:
 my compiler complains about overflow in xlog.c.

Yeah, Peter E. reported this yesterday.  Since Heikki didn't do
anything about that yet, I pushed your fix.

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] Compiler warning in master branch

[Kevin Grittner]

David Rowley dgrowle...@gmail.com wrote: On Mon, Nov 10, 2014 at 4:31 PM, 
Peter Geoghegan p...@heroku.com wrote:


 I see this when I build at -O2 from master's tip:

 brin_revmap.c: In function ‘brinRevmapExtend’:
 brin_revmap.c:113:14: error: variable ‘mapBlk’ set but not used
 [-Werror=unused-but-set-variable]
  BlockNumber mapBlk;
  ^

 It would appear just to need the attached.

Confirmed and pushed.

Thanks to both of you!

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Compiler warning in master branch

[David Rowley]

On Mon, Nov 10, 2014 at 4:31 PM, Peter Geoghegan p...@heroku.com wrote:

 I see this when I build at -O2 from master's tip:

 gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
 -Wdeclaration-after-statement -Wendif-labels
 -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
 -fwrapv -fexcess-precision=standard -Werror -I../../../../src/include
 -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2   -c -o
 brin_revmap.o brin_revmap.c -MMD -MP -MF .deps/brin_revmap.Po
 brin_revmap.c: In function ‘brinRevmapExtend’:
 brin_revmap.c:113:14: error: variable ‘mapBlk’ set but not used
 [-Werror=unused-but-set-variable]
   BlockNumber mapBlk;
   ^

 I'm using gcc 4.8.2 here.


It would appear just to need the attached.

Regards

David Rowley
diff --git a/src/backend/access/brin/brin_revmap.c 
b/src/backend/access/brin/brin_revmap.c
index 7f55ded..272c74e 100644
--- a/src/backend/access/brin/brin_revmap.c
+++ b/src/backend/access/brin/brin_revmap.c
@@ -110,7 +110,7 @@ brinRevmapTerminate(BrinRevmap *revmap)
 void
 brinRevmapExtend(BrinRevmap *revmap, BlockNumber heapBlk)
 {
-   BlockNumber mapBlk;
+   BlockNumber mapBlk PG_USED_FOR_ASSERTS_ONLY;
 
mapBlk = revmap_extend_and_get_blkno(revmap, heapBlk);
 

-- 
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] compiler warning in UtfToLocal and LocalToUtf (conv.c)

[Tom Lane]

Karol Trzcionka karl...@gmail.com writes:
 in the current master head (4cbe3ac3e86790d05c569de4585e5075a62a9b41),
 I've noticed the compiler warnings in src/backend/utils/mb/conv.c

Hm, what compiler version are you using?  I've never seen such a warning
(and that code hasn't changed in some time).

I agree the code could stand to be fixed, but I'm just curious as to
why this is only being seen now.

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] compiler warning in UtfToLocal and LocalToUtf (conv.c)

[Karol Trzcionka]

W dniu 19.07.2013 02:42, Tom Lane pisze:
 Hm, what compiler version are you using? I've never seen such a
 warning (and that code hasn't changed in some time). 
gcc version 4.8.1 20130612 (Red Hat 4.8.1-2) (GCC)
Regards,
Karol


-- 
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] compiler warning

[Bruce Momjian]

Magnus Hagander wrote:
 On Thu, Feb 3, 2011 at 04:40, Bruce Momjian br...@momjian.us wrote:
  I am seeing the following compiler warning for the past few days:
 
  ? ? ? ?basebackup.c:213: warning: variable `ptr' might be clobbered by
  ? ? ? ?`longjmp' or `vfork'
 
  and I see this comment in the file:
 
  ? ? ? ?/*
  ? ? ? ? * Actually do a base backup for the specified tablespaces.
  ? ? ? ? *
  ? ? ? ? * This is split out mainly to avoid complaints about variable 
  might be
  ? ? ? ? * clobbered by longjmp from stupider versions of gcc.
  ? ? ? ? */
 
  Seems that isn't working as expected. ?I am using:
 
  ? ? ? ?gcc version 2.95.3 20010315 (release)
 
  with -O1.
 
 This is the same warning Tom fixed earlier. I have no idea what
 actually causes it :(
 
 I think it's somehow caused by the PG_ENSURE_ERROR_CLEANUP  stuff.
 Does it go away if you break everything inside the if
 (opt-includewal) into it's own function? (Or at least everything
 except the pq_putemptymessage() call, because moving that would make
 the code very confusing)

I added the attached C comment so we know why the warning is generated. 
We can remove it later if we want, but I want to have it if we get
reports about 9.1 problems.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index b5cda50..d94b61f 100644
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
*** perform_base_backup(basebackup_options *
*** 217,222 
--- 217,228 
  ptr.xlogid = logid;
  ptr.xrecoff = logseg * XLogSegSize + TAR_SEND_SIZE * i;
  
+ /*
+  *	Some old compilers, e.g. 2.95.3/x86, think that passing
+  *	a struct in the same function as a longjump might clobber
+  *	a variable.  bjm 2011-02-04
+  *	http://lists.apple.com/archives/xcode-users/2003/Dec//msg00051.html
+  */
  XLogRead(buf, ptr, TAR_SEND_SIZE);
  if (pq_putmessage('d', buf, TAR_SEND_SIZE))
  	ereport(ERROR,

-- 
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] compiler warning

[Magnus Hagander]

On Thu, Feb 3, 2011 at 04:40, Bruce Momjian br...@momjian.us wrote:
 I am seeing the following compiler warning for the past few days:

        basebackup.c:213: warning: variable `ptr' might be clobbered by
        `longjmp' or `vfork'

 and I see this comment in the file:

        /*
         * Actually do a base backup for the specified tablespaces.
         *
         * This is split out mainly to avoid complaints about variable might 
 be
         * clobbered by longjmp from stupider versions of gcc.
         */

 Seems that isn't working as expected.  I am using:

        gcc version 2.95.3 20010315 (release)

 with -O1.

This is the same warning Tom fixed earlier. I have no idea what
actually causes it :(

I think it's somehow caused by the PG_ENSURE_ERROR_CLEANUP  stuff.
Does it go away if you break everything inside the if
(opt-includewal) into it's own function? (Or at least everything
except the pq_putemptymessage() call, because moving that would make
the code very confusing)


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

-- 
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] compiler warning

[Tom Lane]

Jeff Davis pg...@j-davis.com writes:
 A little while ago, I saw a compiler warning creep in:
 In file included from gram.y:11202:
 scan.c: In function ‘yy_try_NUL_trans’:
 scan.c:15765: warning: unused variable ‘yyg’

Yeah, this is a bogosity in the current release of flex.
http://sourceforge.net/tracker/index.php?func=detailaid=2820457group_id=97492atid=618177

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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Zdenek Kotala]

Tom Lane píše v čt 28. 05. 2009 v 11:42 -0400:
 Zdenek Kotala zdenek.kot...@sun.com writes:
  I attached another cleanup patch which fixes following warnings reported
  by Sun Studio:
 
 I'm not too impressed with any of these.  The proposed added
 initializers just increase future maintenance effort without solving
 any real problem (since the variables are required by C standard to
 initialize to zero). 

Agree.

  The proposed signature change on psql_completion
 is going to replace a warning on your system with outright failures on
 other people's.

I check readline and definition is still same at least from 5.0 version.
I'm not still understand why it should failure on other systems. I
looked on revision 1.30 of the file and there is only readline-4.2
support mentioned. Is readline 4.2 the problem?

Zdenek


-- 
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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Zdenek Kotala]

Tom Lane píše v čt 28. 05. 2009 v 11:57 -0400:
 ).
 
 AFAICS, Sun's compiler is just too stupid and shouldn't be emitting
 this warning.  Perhaps the right response is to file a bug report
 against the compiler.

I checked it and it is already know bug. It is new lint style check in
Sun Studio 12. Unfortunately, problem is that current workflow does not
allow to detect if code is dead or not in the verification phase. Next
sun studio release could fix it.

Zdenek


-- 
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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Tom Lane]

Zdenek Kotala zdenek.kot...@sun.com writes:
 Tom Lane píše v čt 28. 05. 2009 v 11:42 -0400:
 The proposed signature change on psql_completion
 is going to replace a warning on your system with outright failures on
 other people's.

 I check readline and definition is still same at least from 5.0 version.
 I'm not still understand why it should failure on other systems. I
 looked on revision 1.30 of the file and there is only readline-4.2
 support mentioned. Is readline 4.2 the problem?

[ pokes around... ]  Actually I think the reason it's like this is that
very old versions of readline have completion_matches() rather than
rl_completion_matches(), and the former is declared to take char * not
const char *.  So it still would compile, you'd just get cast-away-const
warnings.  Which is probably okay considering that hardly anyone is
likely to still be using such old readline libs anyway.

We could try experimenting with that after we branch for 8.5.  I'm not
eager to fool with it in late beta, as we'd be invalidating any port
testing already done.

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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Michael Meskes]

On Thu, May 28, 2009 at 11:11:20AM +0200, Zdenek Kotala wrote:
 I attached another cleanup patch which fixes following warnings reported
 by Sun Studio:
 ...
 preproc.c, line 39569: warning: pointer expression or its operand do not 
 point to the same object yyerror_range, result is undefined and non-portable 
 ...
 Following list is still unfixed plus see my comments:
 
 gram.c, line 28487: warning: pointer expression or its operand do not point 
 to the same object yyerror_range, result is undefined and non-portable 
 ...

These two should be the same, both coming from bison. Both files are
auto-generated, thus it might be bison that has to be fixed to remove this
warning. Given that I didn't find any mentioning of preproc in your patch I
suppose it just hit the wrong list though.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: mes...@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

-- 
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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Zdenek Kotala]

Michael Meskes píše v čt 28. 05. 2009 v 13:33 +0200:
 On Thu, May 28, 2009 at 11:11:20AM +0200, Zdenek Kotala wrote:
  I attached another cleanup patch which fixes following warnings reported
  by Sun Studio:
  ...
  preproc.c, line 39569: warning: pointer expression or its operand do not 
  point to the same object yyerror_range, result is undefined and 
  non-portable 
  ...
  Following list is still unfixed plus see my comments:
  
  gram.c, line 28487: warning: pointer expression or its operand do not 
  point to the same object yyerror_range, result is undefined and 
  non-portable 
  ...
 
 These two should be the same, both coming from bison. Both files are
 auto-generated, thus it might be bison that has to be fixed to remove this
 warning. 

yeah it is generated, but question is if generated code is valid or it
is bug in bison. If it bison bug we need to care about it. There is the
code:

  yyerror_range[1] = yylloc;
  /* Using YYLLOC is tempting, but would change the location of
 the look-ahead.  YYLOC is available though.  */
  YYLLOC_DEFAULT (yyloc, (yyerror_range - 1), 2);
  *++yylsp = yyloc;

Problem is with YYLLOC_DEFAULT. When I look on macro definition 

#define YYLLOC_DEFAULT(Current, Rhs, N)  \
  Current.first_line   = Rhs[1].first_line;  \
  Current.first_column = Rhs[1].first_column;\
  Current.last_line= Rhs[N].last_line;   \
  Current.last_column  = Rhs[N].last_column;

It seems to me that it is OK, because 1 is used as a index which finally
point on yyerror_range[0]. 


 Given that I didn't find any mentioning of preproc in your patch I
 suppose it just hit the wrong list though.

I'm sorry copy paste error. Yeah, I did not fix preproc too.

Zdenek



-- 
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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Michael Meskes]

On Thu, May 28, 2009 at 01:51:07PM +0200, Zdenek Kotala wrote:
 Problem is with YYLLOC_DEFAULT. When I look on macro definition 
 
 #define YYLLOC_DEFAULT(Current, Rhs, N)  \
   Current.first_line   = Rhs[1].first_line;  \
   Current.first_column = Rhs[1].first_column;\
   Current.last_line= Rhs[N].last_line;   \
   Current.last_column  = Rhs[N].last_column;
 
 It seems to me that it is OK, because 1 is used as a index which finally
 point on yyerror_range[0]. 

Wait, this is the bison definition. Well to be more precise the bison
definition in your bison version. Mine is different:

# define YYLLOC_DEFAULT(Current, Rhs, N)\
do  \
  if (YYID (N))\
{   \
  (Current).first_line   = YYRHSLOC (Rhs, 1).first_line;\
  (Current).first_column = YYRHSLOC (Rhs, 1).first_column;  \
  (Current).last_line= YYRHSLOC (Rhs, N).last_line; \
  (Current).last_column  = YYRHSLOC (Rhs, N).last_column;   \
}   \
  else  \
{   \
  (Current).first_line   = (Current).last_line   =  \
YYRHSLOC (Rhs, 0).last_line;\
  (Current).first_column = (Current).last_column =  \
YYRHSLOC (Rhs, 0).last_column;  \
   }   \
while (YYID (0))

Having said that, it doesn't really matter as we redefine the macro:

#define YYLLOC_DEFAULT(Current, Rhs, N) \
do { \
if (N) \
(Current) = (Rhs)[1]; \
else \
(Current) = (Rhs)[0]; \
} while (0)

I have to admit that those version look strikingly unsimilar to me. There is no
reference to Rhs[N] in our macro at all. But then I have no idea whether this
is needed.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: mes...@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

-- 
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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Zdenek Kotala]

Michael Meskes píše v čt 28. 05. 2009 v 14:47 +0200:
 On Thu, May 28, 2009 at 01:51:07PM +0200, Zdenek Kotala wrote:
  Problem is with YYLLOC_DEFAULT. When I look on macro definition 
  
  #define YYLLOC_DEFAULT(Current, Rhs, N)  \
Current.first_line   = Rhs[1].first_line;  \
Current.first_column = Rhs[1].first_column;\
Current.last_line= Rhs[N].last_line;   \
Current.last_column  = Rhs[N].last_column;
  
  It seems to me that it is OK, because 1 is used as a index which finally
  point on yyerror_range[0]. 
 
 Wait, this is the bison definition. Well to be more precise the bison
 definition in your bison version. Mine is different:

I took it from documentation. I have same as you in generated code.

 # define YYLLOC_DEFAULT(Current, Rhs, N)\
 do  \
   if (YYID (N))\
 {   \
   (Current).first_line   = YYRHSLOC (Rhs, 1).first_line;\
   (Current).first_column = YYRHSLOC (Rhs, 1).first_column;  \
   (Current).last_line= YYRHSLOC (Rhs, N).last_line; \
   (Current).last_column  = YYRHSLOC (Rhs, N).last_column;   \
 }   \
   else  \
 {   \
   (Current).first_line   = (Current).last_line   =  \
 YYRHSLOC (Rhs, 0).last_line;\
   (Current).first_column = (Current).last_column =  \
 YYRHSLOC (Rhs, 0).last_column;  \
}   \
 while (YYID (0))
 
 Having said that, it doesn't really matter as we redefine the macro:
 
 #define YYLLOC_DEFAULT(Current, Rhs, N) \
 do { \
 if (N) \
 (Current) = (Rhs)[1]; \
 else \
 (Current) = (Rhs)[0]; \
 } while (0)
 
 I have to admit that those version look strikingly unsimilar to me. There is 
 no
 reference to Rhs[N] in our macro at all. But then I have no idea whether this
 is needed.

Current is only int. See gramparse.h. I think we could rewrite it this
way:

#define YYLLOC_DEFAULT(Current, Rhs, N) \
do { \
if (N) \
(Current) = (Rhs)[1]; \
else \
(Current) = (Rhs)[N]; \
} while (0)

It is same result and compiler is quite.

Zdenek






-- 
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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Tom Lane]

Zdenek Kotala zdenek.kot...@sun.com writes:
 I attached another cleanup patch which fixes following warnings reported
 by Sun Studio:

I'm not too impressed with any of these.  The proposed added
initializers just increase future maintenance effort without solving
any real problem (since the variables are required by C standard to
initialize to zero).  The proposed signature change on psql_completion
is going to replace a warning on your system with outright failures on
other people's.

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] Compiler warning cleanup - unitilized const variables, pointer type mismatch

[Tom Lane]

Michael Meskes mes...@postgresql.org writes:
 I have to admit that those version look strikingly unsimilar to me. There is 
 no
 reference to Rhs[N] in our macro at all. But then I have no idea whether this
 is needed.

The default version of the macro is intended to track both the starting
and ending locations of every construct.  Our simplified version only
tracks the starting locations.  The inputs are RHS[k], the location
values for the constituent elements of the current production, and
the output is the location value for the construct being formed.
In the default version, you naturally want to copy the start of
RHS[1] and the end of RHS[N], where N is the number of production
elements.  In ours, we just copy the location of RHS[1].  However,
both macros need a special case for empty productions (with N = 0).

AFAICS, Sun's compiler is just too stupid and shouldn't be emitting
this warning.  Perhaps the right response is to file a bug report
against the compiler.

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] Compiler warning

[Heikki Linnakangas]

Peter Eisentraut wrote:
Note that applying this patch would introduce a double-translation issue of 
the sort that you had complained about a while ago.  I'm unsure which way to 
proceed here.


Hmm, the patch looks fine to me. The strings are marked with 
gettext_noop() in the array, and passed through _() when used in errmsg.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

--
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] Compiler warning

[Peter Eisentraut]

On Thursday 21 May 2009 10:01:59 Heikki Linnakangas wrote:
 Peter Eisentraut wrote:
  Note that applying this patch would introduce a double-translation issue
  of the sort that you had complained about a while ago.  I'm unsure which
  way to proceed here.

 Hmm, the patch looks fine to me. The strings are marked with
 gettext_noop() in the array, and passed through _() when used in errmsg.

But his patch changes that to

errhint(%s, _(wentry-drophint_msg))

so it ends up being run through gettext twice.  Which has recently been raised 
as a concern.  Both of these competing issues -- the compiler warning and 
double translation -- appear to be minor in practice, but we apparently have 
to make a choice which one we plan to fix now and in the future.

-- 
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] Compiler warning

[Tom Lane]

Peter Eisentraut pete...@gmx.net writes:
 On Thursday 21 May 2009 10:01:59 Heikki Linnakangas wrote:
 Hmm, the patch looks fine to me. The strings are marked with
 gettext_noop() in the array, and passed through _() when used in errmsg.

 But his patch changes that to

 errhint(%s, _(wentry-drophint_msg))

 so it ends up being run through gettext twice.

How so ?  errhint only passes its first argument through gettext.

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] Compiler warning

[Peter Eisentraut]

On Thursday 21 May 2009 14:29:51 Tom Lane wrote:
 Peter Eisentraut pete...@gmx.net writes:
  On Thursday 21 May 2009 10:01:59 Heikki Linnakangas wrote:
  Hmm, the patch looks fine to me. The strings are marked with
  gettext_noop() in the array, and passed through _() when used in errmsg.
 
  But his patch changes that to
 
  errhint(%s, _(wentry-drophint_msg))
 
  so it ends up being run through gettext twice.

 How so ?  errhint only passes its first argument through gettext.

Yeah, not thinking clearly.  So I guess this patch is OK if people care about 
that warning.

-- 
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] Compiler warning

[Heikki Linnakangas]

Fujii Masao wrote:

I encountered the following compiler warning in 8.4dev.
Attached is the patch to fix this problem. Is this worth committing?

--
tablecmds.c: In function 'DropErrorMsgWrongType':
tablecmds.c:606: warning: format not a string literal and no format arguments
--


Hmm, it is a false alarm, but would be nice to have a warning-free 
build. I don't see that warning here on gcc 4.3.3 by default, but I do 
when I set -Wformat-security. I presume you had that set as well.


Applied.

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

--
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] Compiler warning

[Tom Lane]

Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:
 Hmm, it is a false alarm, but would be nice to have a warning-free 
 build. I don't see that warning here on gcc 4.3.3 by default, but I do 
 when I set -Wformat-security. I presume you had that set as well.

Would it be worth having configure probe for that switch and add it to
CFLAGS if available?

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] Compiler warning

[Peter Eisentraut]

On Wednesday 20 May 2009 16:24:21 Tom Lane wrote:
 Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:
  Hmm, it is a false alarm, but would be nice to have a warning-free
  build. I don't see that warning here on gcc 4.3.3 by default, but I do
  when I set -Wformat-security. I presume you had that set as well.

 Would it be worth having configure probe for that switch and add it to
 CFLAGS if available?

Note that applying this patch would introduce a double-translation issue of 
the sort that you had complained about a while ago.  I'm unsure which way to 
proceed here.

-- 
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] Compiler warning with 'fast' variable

[Tom Lane]

Bruce Momjian br...@momjian.us writes:
 Any idea why I am seeing this warning with the new pg_start_backup()
 'fast' flag?

   xlog.c:6917: warning: variable `fast' might be clobbered by
   `longjmp' or `vfork'

 The line is in a PG_ENSURE_ERROR_CLEANUP() block.  This is with gcc
 version 2.95.3.

That's pretty bizarre --- I don't see it here with gcc 2.95.3,
and there is no reason for such a warning to appear on a variable
that isn't changed during the function.

We could stick a volatile on it but I'd like to find out why this
particular variable seems to need that.

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] Compiler warning with 'fast' variable

[Bruce Momjian]

Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  Any idea why I am seeing this warning with the new pg_start_backup()
  'fast' flag?
 
  xlog.c:6917: warning: variable `fast' might be clobbered by
  `longjmp' or `vfork'
 
  The line is in a PG_ENSURE_ERROR_CLEANUP() block.  This is with gcc
  version 2.95.3.
 
 That's pretty bizarre --- I don't see it here with gcc 2.95.3,
 and there is no reason for such a warning to appear on a variable
 that isn't changed during the function.
 
 We could stick a volatile on it but I'd like to find out why this
 particular variable seems to need that.

You ready for this;  changing 'bool' to 'int' supressed the warning:

int fast = PG_GETARG_BOOL(1);

Using 'char' returns the warning.  Changing the assignment to 'true'
also fixes it:

boolfast = true;

This also generates no warning about longjmp:

boolfast = PG_GETARG_TEXT(1);

No warning here either:

boolfast = (bool) PG_GETARG_DATUM(0);

This generates the warning:

boolfast = ((bool) ((bool) (PG_GETARG_DATUM(1)) != 0));

This does not:

boolfast = (bool) (PG_GETARG_DATUM(1) != 0);

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

-- 
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] Compiler warning with 'fast' variable

[Tom Lane]

Bruce Momjian br...@momjian.us writes:
 Tom Lane wrote:
 We could stick a volatile on it but I'd like to find out why this
 particular variable seems to need that.

 You ready for this;  changing 'bool' to 'int' supressed the warning:
   int fast = PG_GETARG_BOOL(1);

Well, that's a compiler bug :-(.  Probably platform-specific, too,
which is why I don't see it on HPPA.

I think that the above variant is the least ugly of the alternatives
you show as working, and definitely less ugly than plastering volatile
on it.

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] Compiler warning with 'fast' variable

[Bruce Momjian]

Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  Tom Lane wrote:
  We could stick a volatile on it but I'd like to find out why this
  particular variable seems to need that.
 
  You ready for this;  changing 'bool' to 'int' supressed the warning:
  int fast = PG_GETARG_BOOL(1);
 
 Well, that's a compiler bug :-(.  Probably platform-specific, too,
 which is why I don't see it on HPPA.
 
 I think that the above variant is the least ugly of the alternatives
 you show as working, and definitely less ugly than plastering volatile
 on it.

Well, let's leave it alone and see if anyone else find it;  I can mask
it on my end.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

-- 
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] Compiler warning in ecpglib/execute.c

[Michael Meskes]

On Tue, Feb 03, 2009 at 04:06:39PM -0300, Alvaro Herrera wrote:
 Note that spoonbill is still red.  This animal is special because it
 uses some peculiar malloc flags.

Not just spoonbill, five of them are. Without access to a failing machine this
is difficult to debug. I found some missing error checks in that library that
might explain the segfaults, but I still don't see why the the test is failing.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: mes...@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

-- 
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] Compiler warning in ecpglib/execute.c

[Michael Meskes]

On Mon, Feb 02, 2009 at 02:56:18PM -0500, Tom Lane wrote:
 CVS HEAD is producing
 ...

Thanks for the note, I missed this copypaste error of mine. Fixed in HEAD.
This should alos make the buildfarm green again.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: mes...@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

-- 
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] Compiler warning in ecpglib/execute.c

[Alvaro Herrera]

Michael Meskes wrote:
 On Mon, Feb 02, 2009 at 02:56:18PM -0500, Tom Lane wrote:
  CVS HEAD is producing
  ...
 
 Thanks for the note, I missed this copypaste error of mine. Fixed in HEAD.
 This should alos make the buildfarm green again.

Note that spoonbill is still red.  This animal is special because it
uses some peculiar malloc flags.

-- 
Alvaro Herrerahttp://www.advogato.org/person/alvherre
La persona que no quería pecar / estaba obligada a sentarse
 en duras y empinadas sillas/ desprovistas, por cierto
 de blandos atenuantes  (Patricio Vogel)

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