Обсуждение: Re: [HACKERS] Compiler warning in costsize.c
On Tue, Apr 4, 2017 at 9:42 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Apr 5, 2017 at 2:54 AM, Tom Lane <tgl@sss.pgh.pa.us> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On Fri, Apr 7, 2017 at 12:38 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Apr 4, 2017 at 9:42 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Wed, Apr 5, 2017 at 2:54 AM, Tom Lane <tgl@sss.pgh.pa.us> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Michael Paquier <michael.paquier@gmail.com> 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_CHECKINGAssert(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
On 8 April 2017 at 04:42, Tom Lane <tgl@sss.pgh.pa.us> 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
David Rowley <david.rowley@2ndquadrant.com> writes:
> On 8 April 2017 at 04:42, Tom Lane <tgl@sss.pgh.pa.us> 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
aresubqueries */ 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
On Mon, Apr 10, 2017 at 9:05 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
On Mon, Apr 10, 2017 at 8:30 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Apr 10, 2017 at 9:05 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
Tom Lane <tgl@sss.pgh.pa.us> 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 withthe consequences of." --Skud's Meta-Law
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 10, 2017 at 8:30 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Mon, Apr 10, 2017 at 9:05 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) 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
On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) 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
On Tue, Apr 11, 2017 at 4:02 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) 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
On 11 April 2017 at 12:53, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Apr 11, 2017 at 4:02 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) 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