Aggregate leads to superfluous projection from the scan

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&amp;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

On Fri, Jul 8, 2022 at 10:32 PM Zhihong Yu <zyu@yugabyte.com> wrote:

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&amp;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

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.

postgres=# explain (analyze, verbose) /*+ IndexScan(idx) */select
count(fire_year) from fires t where objectid = 20;

QUERY PLAN

------------------------------------------------------------------------------------------------------------------------------------------------------------------

------------------------------------

Aggregate (cost=8.31..8.32 rows=1 width=8) (actual time=0.029..0.030
rows=1 loops=1)

Output: count(fire_year)

-> Index Scan using fires_pkey on public.fires t (cost=0.29..8.31
rows=1 width=4) (actual time=0.022..0.023 rows=1 loops=1)

Output: objectid, fire_name, fire_year, discovery_date,
discovery_time, stat_cause_descr, fire_size, fire_size_class, latitude,
longitude, state, county,

discovery_date_j, discovery_date_d

Index Cond: (t.objectid = 20)

Planning Time: 0.076 ms

Execution Time: 0.059 ms

(7 rows)

Index-only.

postgres=# explain (analyze, verbose) /*+ IndexScan(idx) */select
count(fire_year) from fires t where fire_year = 20;

QUERY PLAN

-------------------------------------------------------------------------------------------------------------------------------

Aggregate (cost=8.31..8.32 rows=1 width=8) (actual time=0.026..0.027
rows=1 loops=1)

Output: count(fire_year)

-> Index Only Scan using idx on public.fires t (cost=0.29..8.31 rows=1
width=4) (actual time=0.023..0.024 rows=0 loops=1)

Output: fire_year

Index Cond: (t.fire_year = 20)

Heap Fetches: 0

Planning Time: 0.140 ms

Execution Time: 0.052 ms

(8 rows)

Index Scans

------------

postgres=# explain (analyze, verbose) select count(fire_year) from fires t
where objectid = 20;

Aggregate (cost=8.31..8.32 rows=1 width=8) (actual time=0.030..0.031
rows=1 loops=1)

Output: count(fire_year)

-> Index Scan using fires_pkey on public.fires t (cost=0.29..8.31
rows=1 width=4) (actual time=0.021..0.023 rows=1 loops=1)

Output: objectid, fire_name, fire_year, discovery_date,
discovery_time, stat_cause_descr, fire_size, fire_size_class, latitude,
longitude, state, county,

discovery_date_j, discovery_date_d

Index Cond: (t.objectid = 20)

Planning Time: 0.204 ms

Execution Time: 0.072 ms

(7 rows)

Seq scans.

----------

postgres=# explain (analyze, verbose) select count(fire_year) from fires t;

Aggregate (cost=1791.00..1791.01 rows=1 width=8) (actual
time=13.172..13.174 rows=1 loops=1)

Output: count(fire_year)

-> Seq Scan on public.fires t (cost=0.00..1541.00 rows=100000 width=4)
(actual time=0.007..6.500 rows=100000 loops=1)

Output: objectid, fire_name, fire_year, discovery_date,
discovery_time, stat_cause_descr, fire_size, fire_size_class, latitude,
longitude, state, county,

discovery_date_j, discovery_date_d

Planning Time: 0.094 ms

Execution Time: 13.201 ms

(6 rows)

--
Ibrar Ahmed

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

On Fri, Jul 8, 2022 at 12:48 PM Zhihong Yu <zyu@yugabyte.com> wrote:

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

Hi, Tom:
I was looking at the following comment in createplan.c :

* For table scans, rather than using the relation targetlist (which is
* only those Vars actually needed by the query), we prefer to generate
a
* tlist containing all Vars in order. This will allow the executor to
* optimize away projection of the table tuples, if possible.

Maybe you can give me some background on the above decision.

Thanks

Zhihong Yu <zyu@yugabyte.com> writes:

I was looking at the following comment in createplan.c :

* For table scans, rather than using the relation targetlist (which is
* only those Vars actually needed by the query), we prefer to generate
a
* tlist containing all Vars in order. This will allow the executor to
* optimize away projection of the table tuples, if possible.

Maybe you can give me some background on the above decision.

Look into execScan.c and note that it skips doing ExecProject() if the
scan node's targetlist exactly matches the table's tuple descriptor.
And particularly this comment:

* We can avoid a projection step if the requested tlist exactly matches
* the underlying tuple type. If so, we just set ps_ProjInfo to NULL.
* Note that this case occurs not only for simple "SELECT * FROM ...", but
* also in most cases where there are joins or other processing nodes above
* the scan node, because the planner will preferentially generate a matching
* tlist.

regards, tom lane