Обсуждение: Aggregate leads to superfluous projection from the scan

Hi,

Here is the query which involves aggregate on a single column:

https://dbfiddle.uk/?rdbms=postgres_13&fiddle=44bfd8f6b6b5aad34d00d449c04c5a96

As you can see from `Output:`, there are many columns added which are not needed by the query executor.

I wonder if someone has noticed this in the past.

If so, what was the discussion around this topic ?

Thanks

On Fri, Jul 8, 2022 at 9:40 AM Zhihong Yu <zyu@yugabyte.com> wrote:

Hi,

Here is the query which involves aggregate on a single column:

https://dbfiddle.uk/?rdbms=postgres_13&fiddle=44bfd8f6b6b5aad34d00d449c04c5a96

As you can see from `Output:`, there are many columns added which are not needed by the query executor.

I wonder if someone has noticed this in the past.

If so, what was the discussion around this topic ?

Thanks

Hi,

With the patch, I was able to get the following output:

 explain (analyze, verbose) /*+ IndexScan(t) */select count(fire_year) from fires t where objectid <= 2000000;

                                                              QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------
 Aggregate  (cost=119.00..119.01 rows=1 width=8) (actual time=9.453..9.453 rows=1 loops=1)
   Output: count(fire_year)
   ->  Index Scan using fires_pkey on public.fires t  (cost=0.00..116.50 rows=1000 width=4) (actual time=9.432..9.432 rows=0 loops=1)
         Output: fire_year
         Index Cond: (t.objectid <= 2000000)
 Planning Time: 52.598 ms
 Execution Time: 13.082 ms

Please pay attention to the column list after `Output:`

Tom:

Can you take a look and let me know what I may have missed ?

Thanks

Ibrar Ahmed <ibrar.ahmad@gmail.com> writes:
> I give a quick look and I think in case whenever data is extracted from the
> heap it shows all the columns. Therefore when columns are extracted from
> the index only it shows the indexed column only.

This is operating as designed, and I don't think that the proposed
patch is an improvement.  The point of use_physical_tlist() is that
returning all the columns is cheaper because it avoids a projection
step.  That's true for any case where we have to fetch the heap
tuple, so IndexScan is included though IndexOnlyScan is not.

Now, that's something that was true a decade or more ago.
There's been considerable discussion recently about cases where
it's not true anymore, for example with columnar storage or FDWs,
and so we ought to invent a way to prevent createplan.c from
doing it when it would be counterproductive.  But just summarily
turning it off is not an improvement.

                        regards, tom lane

On Fri, Jul 8, 2022 at 12:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Ibrar Ahmed <ibrar.ahmad@gmail.com> writes:
> I give a quick look and I think in case whenever data is extracted from the
> heap it shows all the columns. Therefore when columns are extracted from
> the index only it shows the indexed column only.

This is operating as designed, and I don't think that the proposed
patch is an improvement.  The point of use_physical_tlist() is that
returning all the columns is cheaper because it avoids a projection
step.  That's true for any case where we have to fetch the heap
tuple, so IndexScan is included though IndexOnlyScan is not.

Now, that's something that was true a decade or more ago.
There's been considerable discussion recently about cases where
it's not true anymore, for example with columnar storage or FDWs,
and so we ought to invent a way to prevent createplan.c from
doing it when it would be counterproductive.  But just summarily
turning it off is not an improvement.

                        regards, tom lane

Hi,

In createplan.c, there is `change_plan_targetlist`

Plan *

change_plan_targetlist(Plan *subplan, List *tlist, bool tlist_parallel_safe)

But it doesn't have `Path` as parameter.

So I am not sure whether the check of non-returnable columns should be done in change_plan_targetlist().

bq. for example with columnar storage or FDWs,

Yeah. The above is the case where I want to optimize.

Cheers