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

[HACKERS] Compiler warning in costsize.c

[Michael Paquier]

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,
-- 
Michael


costsize-fix-warning.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