Compiler warning in costsize.c

Started by Michael Paquieralmost 9 years ago18 messages

michael.paquier@gmail.com

almost 9 years ago

1 attachment(s)

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

Attachments:

costsize-fix-warning.patchapplication/octet-stream; name=costsize-fix-warning.patchDownload

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index ed07e2f655..b79b0d975a 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -4517,7 +4517,9 @@ set_subquery_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 {
 	PlannerInfo *subroot = rel->subroot;
 	RelOptInfo *sub_final_rel;
+#ifdef USE_ASSERT_CHECKING
 	RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
+#endif
 	ListCell   *lc;
 
 	/* Should only be applied to base relations that are subqueries */
@@ -4637,7 +4639,9 @@ set_function_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 void
 set_tablefunc_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 {
+#ifdef USE_ASSERT_CHECKING
 	RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
+#endif
 
 	/* Should only be applied to base relations that are functions */
 	Assert(rel->relid > 0);

David Rowley

david.rowley@2ndquadrant.com

almost 9 years ago

In reply to: Michael Paquier (#1)

1 attachment(s)

Re: Compiler warning in costsize.c

On 4 April 2017 at 16:22, Michael Paquier <michael.paquier@gmail.com> 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

Attachments:

costsize-fix-warning_drowley.patchapplication/octet-stream; name=costsize-fix-warning_drowley.patchDownload

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index a2093ac..7683ea9 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -4487,12 +4487,12 @@ set_subquery_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 {
 	PlannerInfo *subroot = rel->subroot;
 	RelOptInfo *sub_final_rel;
-	RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
 	ListCell   *lc;
+#ifdef USE_ASSERT_CHECKING
+	RangeTblEntry *rte;
 
 	/* 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
@@ -4607,11 +4607,12 @@ set_function_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 void
 set_tablefunc_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 {
-	RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
+#ifdef USE_ASSERT_CHECKING
+	RangeTblEntry *rte;
 
 	/* Should only be applied to base relations that are functions */
 	Assert(rel->relid > 0);
-#ifdef USE_ASSERT_CHECKING
+
 	rte = planner_rt_fetch(rel->relid, root);
 	Assert(rte->rtekind == RTE_TABLEFUNC);
 #endif

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: David Rowley (#2)

Re: Compiler warning in costsize.c

On Tue, Apr 4, 2017 at 7:03 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:

On 4 April 2017 at 16:22, Michael Paquier <michael.paquier@gmail.com> 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

Tom Lane

tgl@sss.pgh.pa.us

almost 9 years ago

In reply to: Michael Paquier (#1)

Re: Compiler warning in costsize.c

Michael Paquier <michael.paquier@gmail.com> 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

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Tom Lane (#4)

Re: Compiler warning in costsize.c

On Wed, Apr 5, 2017 at 2:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael.paquier@gmail.com> 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

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Michael Paquier (#5)

1 attachment(s)

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

Attachments:

costsize-warning-fix-2.patchapplication/octet-stream; name=costsize-warning-fix-2.patchDownload

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index ed07e2f..727d115 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -4520,13 +4520,13 @@ set_subquery_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 	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
 
+	/* Should only be applied to base relations that are subqueries */
+	Assert(rel->relid > 0 && rte->rtekind == RTE_SUBQUERY);
+
 	/*
 	 * Copy raw number of output rows from subquery.  All of its paths should
 	 * have the same output rowcount, so just look at cheapest-total.
@@ -4639,13 +4639,13 @@ set_tablefunc_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 {
 	RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
 
-	/* Should only be applied to base relations that are functions */
-	Assert(rel->relid > 0);
 #ifdef USE_ASSERT_CHECKING
 	rte = planner_rt_fetch(rel->relid, root);
-	Assert(rte->rtekind == RTE_TABLEFUNC);
 #endif
 
+	/* Should only be applied to base relations that are functions */
+	Assert(rel->relid > 0 && rte->rtekind == RTE_TABLEFUNC);
+
 	rel->tuples = 100;
 
 	/* Now estimate number of output rows, etc */

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Michael Paquier (#6)

1 attachment(s)

Re: Compiler warning in costsize.c

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

Attachments:

costsize-warning-fix-3.patchapplication/octet-stream; name=costsize-warning-fix-3.patchDownload

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index ed07e2f..3c35968 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -4517,15 +4517,11 @@ set_subquery_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 {
 	PlannerInfo *subroot = rel->subroot;
 	RelOptInfo *sub_final_rel;
-	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
+	Assert((planner_rt_fetch(rel->relid, root))->rtekind == RTE_SUBQUERY);
 
 	/*
 	 * Copy raw number of output rows from subquery.  All of its paths should
@@ -4637,14 +4633,9 @@ set_function_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 void
 set_tablefunc_size_estimates(PlannerInfo *root, RelOptInfo *rel)
 {
-	RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
-
 	/* Should only be applied to base relations that are functions */
 	Assert(rel->relid > 0);
-#ifdef USE_ASSERT_CHECKING
-	rte = planner_rt_fetch(rel->relid, root);
-	Assert(rte->rtekind == RTE_TABLEFUNC);
-#endif
+	Assert((planner_rt_fetch(rel->relid, root))->rtekind == RTE_TABLEFUNC);
 
 	rel->tuples = 100;

Tom Lane

tgl@sss.pgh.pa.us

almost 9 years ago

In reply to: Michael Paquier (#7)

Re: Compiler warning in costsize.c

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

David Rowley

david.rowley@2ndquadrant.com

almost 9 years ago

In reply to: Tom Lane (#8)

Re: Compiler warning in costsize.c

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

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

#10

Tom Lane

tgl@sss.pgh.pa.us

almost 9 years ago

In reply to: David Rowley (#9)

Re: Compiler warning in costsize.c

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

#11

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Tom Lane (#10)

Re: Compiler warning in costsize.c

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

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

#12

Robert Haas

robertmhaas@gmail.com

almost 9 years ago

In reply to: Michael Paquier (#11)

Re: Compiler warning in costsize.c

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

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

#13

Noname

ilmari@ilmari.org

almost 9 years ago

In reply to: Tom Lane (#10)

Re: Compiler warning in costsize.c

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

#14

Tom Lane

tgl@sss.pgh.pa.us

almost 9 years ago

In reply to: Robert Haas (#12)

Re: Compiler warning in costsize.c

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

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

#15

Tom Lane

tgl@sss.pgh.pa.us

almost 9 years ago

In reply to: Noname (#13)

Re: Compiler warning in costsize.c

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

#16

Robert Haas

robertmhaas@gmail.com

almost 9 years ago

In reply to: Tom Lane (#15)

Re: Compiler warning in costsize.c

On Mon, Apr 10, 2017 at 2:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

#17

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Robert Haas (#16)

Re: Compiler warning in costsize.c

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

#18

David Rowley

david.rowley@2ndquadrant.com

over 8 years ago

In reply to: Michael Paquier (#17)

Re: Compiler warning in costsize.c

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