Re: Getting sorted data from foreign server - Mailing list pgsql-hackers

On Wed, Oct 14, 2015 at 7:30 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> The patch uses a factor of 1.1 (10% increase) to multiple the startup and
> total costs in fpinfo for unsorted data.
>
> This change has caused the plans for few queries in the test postgres_fdw to
> change. There is ORDER BY and LIMIT clause in the queries in postgres_fdw
> testcase to keep test outputs sane and consistent. These ORDER BY clauses
> are not being pushed down the foreign servers. I tried using values upto 2
> for this but still the foreign paths with pathkeys won over those without
> pathkeys.

That's great.  Seeing unnecessary sorts disappear from the regression
tests as a result of this patch is, IMHO, pretty cool.  But these
compiler warnings might also be related:

postgres_fdw.c:605:7: error: variable 'rows' is used uninitialized whenever 'if'     condition is false
[-Werror,-Wsometimes-uninitialized]              if (fpinfo->use_remote_estimate)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
postgres_fdw.c:628:13: note: uninitialized use occurs here ...rows,    ^~~~
postgres_fdw.c:605:3: note: remove the 'if' if its condition is always true               if
(fpinfo->use_remote_estimate)              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
postgres_fdw.c:596:15: note: initialize the variable 'rows' to silence this     warning               double
rows;                                  ^                                    = 0.0
 
postgres_fdw.c:605:7: error: variable 'startup_cost' is used uninitialized     whenever 'if' condition is false
[-Werror,-Wsometimes-uninitialized]              if (fpinfo->use_remote_estimate)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
postgres_fdw.c:629:13: note: uninitialized use occurs here ...startup_cost,    ^~~~~~~~~~~~
postgres_fdw.c:605:3: note: remove the 'if' if its condition is always true               if
(fpinfo->use_remote_estimate)              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
postgres_fdw.c:598:21: note: initialize the variable 'startup_cost' to silence     this warning               Cost
     startup_cost;                                           ^                                            = 0.0
 
postgres_fdw.c:605:7: error: variable 'total_cost' is used uninitialized     whenever 'if' condition is false
[-Werror,-Wsometimes-uninitialized]              if (fpinfo->use_remote_estimate)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
postgres_fdw.c:630:13: note: uninitialized use occurs here ...total_cost,    ^~~~~~~~~~
postgres_fdw.c:605:3: note: remove the 'if' if its condition is always true               if
(fpinfo->use_remote_estimate)              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
postgres_fdw.c:599:19: note: initialize the variable 'total_cost' to silence     this warning               Cost
   total_cost;                                         ^                                          = 0.0
 

At least some of those look like actual mistakes.

I would suggest that instead of the regression test you added, you
instead change one of the existing tests to order by a non-pushable
expression that produces the same final output ordering.  The revised
EXPLAIN output shows that the existing tests are already testing that
the sort is getting pushed down; we don't need another test for the
same thing.  Instead, let's adjust one of the tests so that we can
also show that we don't push down ORDER BY clauses when it would be
wrong to do so.  For example, suppose the user specifies a collation
in the ORDER BY clause.

appendOrderByClause could use a header comment, and its definition
line is more than 80 characters, so it should be wrapped.

DEFAULT_FDW_SORT_MULTIPLIER should have a one-line comment, like /* If
no remote estimates, assume a sort costs 10% extra */.  The comment
inside postgresGetForeignPaths shouldn't refer specifically to the 10%
value, in case we change it.  So maybe: "Assume sorting costs
something, so that we won't ask for sorted results when they aren't
useful, but not too much, so that remote sorts will look attractive
when the ordering is useful to us."  Note that "attractive" is missing
a "t" in the patch.

Do you need to ignore ec_has_volatile equivalence classes in
find_em_expr_for_rel?  I bet yes.

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