Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8ySf-0005wU-5a for pgsql-bugs@arkaria.postgresql.org; Fri, 21 Aug 2020 04:15:29 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1k8ySe-0007sj-4g for pgsql-bugs@arkaria.postgresql.org; Fri, 21 Aug 2020 04:15:28 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8ySd-0007sO-Jm for pgsql-bugs@lists.postgresql.org; Fri, 21 Aug 2020 04:15:27 +0000 Received: from mail-ej1-x630.google.com ([2a00:1450:4864:20::630]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1k8ySW-0000eI-Ew for pgsql-bugs@lists.postgresql.org; Fri, 21 Aug 2020 04:15:26 +0000 Received: by mail-ej1-x630.google.com with SMTP id qc22so768261ejb.4 for ; Thu, 20 Aug 2020 21:15:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=FQw+z01rzYAiPFxy7mMeEnqRwYyOqpQ+ctcAPZMfE64=; b=UdSu3oKB+stiw20HA2Ur9Y8qXyI9aRIInlEnNDH9AvDqD6zDvKu4q6unVQ6NtnyYvT zNT2sP4ukKwVUgUwdEF3vIkMG3aMUlz+XKDTxzsgDhEs1Rmq4Dl68bh1dPT+qoao6uZ+ gSp3oTLr6Y9J0B16COgBr00m/cq+ai960rJdG3jscWe9JuM/z8+01PxFTkv1bbTGD6Zg 4f1nkknwCJlIij++3Tp8JRhsy7egrAZYAvrjwpcD2mvX94TesutM1oX3KKFDhGq6zlGK VaTg+pzYoUkUWjRX3akaCKEEhXmPYUwZIrqDXWw/nzIFkIJ++n26NWpwVqAbF+fmUu5O QL0Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=FQw+z01rzYAiPFxy7mMeEnqRwYyOqpQ+ctcAPZMfE64=; b=CmUuGze2StcitILzBAPjl1FPXakCphbDwxu0cbrawsSEAAtzb0htW8JmWL33XblHIM cdPOgz4Sn1pIUjyexU2T2xZ26WZ2/I5JyVl14Qi1RQs0xPowdvxEXRc9Ny+ePliMd282 sIHk4JR+HpHlKYf9YuKY/udQKnQWm/AHMVRW645LwtcaUCfzxgp8BpHxkHeWjHUgAYbX czMgSyRsEGPg8vt7uer1eIJ7K3OTPGOgFJfLWrw5j5dovM0ZXoti7dDFc7TDOlqCEXZg 6ZdClr5bB11n8dHWUX+RCkoIEMc1Al+JMBHUC8uIr7+/hIaOXdKbCcXvhUd/zKq7Hvpa K63w== X-Gm-Message-State: AOAM531KYFUaou1X0ELUQ0j5ydPJgCFT0V3kB+4KQc5P5Tdt7R++6dNJ EG8gvJXDGnzkn3d1ulB9lFNOC78IbeyH90/UzHc= X-Google-Smtp-Source: ABdhPJzvqL56Di2QQ195tjPm7d8gChKr3f601CHQi+VBPwub+XYPi9F3ieWp1UibEQqit9Do/rSZjIZuiCyAQ/yRWkI= X-Received: by 2002:a17:906:ce39:: with SMTP id sd25mr1189595ejb.80.1597983318071; Thu, 20 Aug 2020 21:15:18 -0700 (PDT) MIME-Version: 1.0 References: <16585-9d8c340d23ade8c1@postgresql.org> <87mu2qixx5.fsf@news-spur.riddles.org.uk> <1750212.1597958727@sss.pgh.pa.us> <87tuwxrnzm.fsf@news-spur.riddles.org.uk> <1765056.1597977852@sss.pgh.pa.us> In-Reply-To: From: Andy Fan Date: Fri, 21 Aug 2020 12:15:06 +0800 Message-ID: Subject: Re: BUG #16585: Wrong filtering on a COALESCE field after using GROUPING SETS To: Tom Lane Cc: Andrew Gierth , David Rowley , pavelsivash@gmail.com, PostgreSQL mailing lists Content-Type: multipart/alternative; boundary="00000000000007df0805ad5b7c0c" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000007df0805ad5b7c0c Content-Type: text/plain; charset="UTF-8" On Fri, Aug 21, 2020 at 11:50 AM Andy Fan wrote: > > > On Fri, Aug 21, 2020 at 10:44 AM Tom Lane wrote: > >> Andy Fan writes: >> > On Fri, Aug 21, 2020 at 5:51 AM Andrew Gierth < >> andrew@tao11.riddles.org.uk> >> > wrote: >> >> Unless I'm missing something, it should be safe to reference output >> >> columns that are not mentioned in any grouping set, >> >> > I think such columns usually are aggregation expr, If we want to push >> down >> > a qual which reference to an aggregation expr, we have to push down >> > to having cause, However I am not sure such pushing down really helps. >> >> Well, they can either be aggregates, or functions of the grouping >> columns. You're right that there's not much we can do (today) with >> restrictions on aggregate outputs, but there can be value in pushing >> down restrictions on the other sort. >> >> As an example, consider the regression database's tenk1 table, and >> for argument's sake add >> >> regression=# create index on tenk1 (abs(hundred)); >> CREATE INDEX >> >> Then we can get >> >> regression=# explain select * from (select hundred, ten, abs(hundred) a, >> count(*) c from tenk1 group by 1,2) ss where a = 42; >> QUERY PLAN >> >> >> ------------------------------------------------------------------------------------ >> HashAggregate (cost=225.98..227.18 rows=96 width=20) >> Group Key: tenk1.hundred, tenk1.ten >> -> Bitmap Heap Scan on tenk1 (cost=5.06..225.23 rows=100 width=8) >> Recheck Cond: (abs(hundred) = 42) >> -> Bitmap Index Scan on tenk1_abs_idx (cost=0.00..5.04 >> rows=100 width=0) >> Index Cond: (abs(hundred) = 42) >> (6 rows) >> >> which is a lot cheaper than the pure seqscan you get with no pushed-down >> condition. >> >> One thing that I find curious is that if I alter this example to use >> grouping sets, say >> >> regression=# explain select * from (select hundred, ten, abs(hundred) a, >> count(*) c from tenk1 group by grouping sets (1,2)) ss where a = 42; >> QUERY PLAN >> ----------------------------------------------------------------- >> HashAggregate (cost=495.00..546.65 rows=2 width=20) >> Hash Key: tenk1.hundred >> Hash Key: tenk1.ten >> Filter: (abs(tenk1.hundred) = 42) >> -> Seq Scan on tenk1 (cost=0.00..445.00 rows=10000 width=8) >> (5 rows) >> >> i.e. it's not seeing the abs() condition as pushable below the >> aggregation. I'm not quite sure if that's a necessary restriction >> or a missed optimization. >> >> regards, tom lane >> > > Both of the queries can push down the qual "a = 42" to the > subquery->havingQual > since we have group-by clause, this method unify the process for > aggregation call > and non-aggregation expr. . so it become to > > select .. from (select .. from tenk1 group .. having (abs(hundred) = 2); > > > later in the subquery_planner, we will try to pull the having clause to > where clause. > then the Q2 failed to do so. > > /* > * In some cases we may want to transfer a HAVING clause into > WHERE. We > * cannot do so if the HAVING clause contains aggregates > (obviously) or > * volatile functions (since a HAVING clause is supposed to be > executed > * only once per group). We also can't do this if there are any > nonempty > * grouping sets; moving such a clause into WHERE would > potentially change > * the results, if any referenced column isn't present in all the > grouping > * sets. (If there are only empty grouping sets, then the HAVING > clause > * must be degenerate as discussed below.) > */ > > I'm still trying to understand the comment, though. > > > This should be a correct behavior, we should not push down in the Q2 case. Here is an example: regression=# create table tgs(a int, b int); CREATE TABLE regression=# insert into tgs values(1, 1), (1, 2), (2, 2); INSERT 0 3 regression=# select * from (select a, b, count(*) from tgs group by grouping sets((a), (b))) t where b = 1; a | b | count ---+---+------- | 1 | 1 (1 row) regression=# select * from (select a, b, count(*) from tgs group by grouping sets((a), (b)) having b = 1) t; a | b | count ---+---+------- | 1 | 1 (1 row) regression=# select * from (select a, b, count(*) from tgs where b = 1 group by grouping sets((a), (b)) ) t; a | b | count ---+---+------- 1 | | 1 | 1 | 1 (2 rows) At the same time, our optimizer is smart enough to handle the below case (only 1 set in group sets, which equals group by). regression=# explain select * from (select a, b, count(*) from tgs group by grouping sets((a, b)) ) t where b = 1; QUERY PLAN ----------------------------------------------------------------- GroupAggregate (cost=38.44..38.66 rows=11 width=16) Group Key: tgs.a, tgs.b -> Sort (cost=38.44..38.47 rows=11 width=8) Sort Key: tgs.a -> Seq Scan on tgs (cost=0.00..38.25 rows=11 width=8) Filter: (b = 1) (6 rows) -- Best Regards Andy Fan --00000000000007df0805ad5b7c0c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, Aug 21, 2020 at 11:50 AM Andy= Fan <zhihui.fan1213@gmail.c= om> wrote:


On Fri, Aug 21, 2020 at 10:44 AM To= m Lane <tgl@sss.p= gh.pa.us> wrote:
Andy Fan <zhihui.fan1213@gmail.com> writes:
> On Fri, Aug 21, 2020 at 5:51 AM Andrew Gierth <andrew@tao11.riddles.org.uk>
> wrote:
>> Unless I'm missing something, it should be safe to reference o= utput
>> columns that are not mentioned in any grouping set,

> I think such columns usually are aggregation expr,=C2=A0 If we want to= push down
> a qual which reference to an aggregation expr,=C2=A0 we have to push d= own
> to having cause, However I am not sure such pushing down really helps.=

Well, they can either be aggregates, or functions of the grouping
columns.=C2=A0 You're right that there's not much we can do (today)= with
restrictions on aggregate outputs, but there can be value in pushing
down restrictions on the other sort.

As an example, consider the regression database's tenk1 table, and
for argument's sake add

regression=3D# create index on tenk1 (abs(hundred));
CREATE INDEX

Then we can get

regression=3D# explain select * from (select hundred, ten, abs(hundred) a, = count(*) c from tenk1 group by 1,2) ss where a =3D 42;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0QUERY PLAN=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0
---------------------------------------------------------------------------= ---------
=C2=A0HashAggregate=C2=A0 (cost=3D225.98..227.18 rows=3D96 width=3D20)
=C2=A0 =C2=A0Group Key: tenk1.hundred, tenk1.ten
=C2=A0 =C2=A0->=C2=A0 Bitmap Heap Scan on tenk1=C2=A0 (cost=3D5.06..225.= 23 rows=3D100 width=3D8)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Recheck Cond: (abs(hundred) =3D 42)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0->=C2=A0 Bitmap Index Scan on tenk1_ab= s_idx=C2=A0 (cost=3D0.00..5.04 rows=3D100 width=3D0)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Index Cond: (abs(hun= dred) =3D 42)
(6 rows)

which is a lot cheaper than the pure seqscan you get with no pushed-down condition.

One thing that I find curious is that if I alter this example to use
grouping sets, say

regression=3D# explain select * from (select hundred, ten, abs(hundred) a, = count(*) c from tenk1 group by grouping sets (1,2)) ss where a =3D 42;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0QUERY PLAN=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0
-----------------------------------------------------------------
=C2=A0HashAggregate=C2=A0 (cost=3D495.00..546.65 rows=3D2 width=3D20)
=C2=A0 =C2=A0Hash Key: tenk1.hundred
=C2=A0 =C2=A0Hash Key: tenk1.ten
=C2=A0 =C2=A0Filter: (abs(tenk1.hundred) =3D 42)
=C2=A0 =C2=A0->=C2=A0 Seq Scan on tenk1=C2=A0 (cost=3D0.00..445.00 rows= =3D10000 width=3D8)
(5 rows)

i.e. it's not seeing the abs() condition as pushable below the
aggregation.=C2=A0 I'm not quite sure if that's a necessary restric= tion
or a missed optimization.

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 regards, tom lane

Both of the queries can push down= the qual "a =3D 42"=C2=A0 to the subquery->havingQual=C2=A0
since we have group-by clause,=C2=A0 this method unify the process= for aggregation call
and non-aggregation expr.=C2=A0 .=C2=A0 so = it become to

select .. from (select .. from tenk1 = group ..=C2=A0 having (abs(hundred) =3D 2);=C2=A0 =C2=A0

later in the subquery_planner,=C2=A0 we will try to pull the having = clause to where clause.=C2=A0
then the Q2 failed to do so.=C2=A0<= /div>

/*=C2=A0
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0* In some cases we may want to transfer a HAVING clause into WHERE. We=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* cannot do so if the HAVING clause con= tains aggregates (obviously) or
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* vola= tile functions (since a HAVING clause is supposed to be executed
=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0* only once per group).=C2=A0 We also can't = do this if there are any nonempty
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* gr= ouping sets; moving such a clause into WHERE would potentially change
= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* the results, if any referenced column i= sn't present in all the grouping
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*= sets. =C2=A0(If there are only empty grouping sets, then the HAVING clause=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* must be degenerate as discussed bel= ow.)
*/=C2=A0

I'm still trying t= o understand the comment, though.=C2=A0


=

This should be a correct behavior,= =C2=A0 we should not push down in the Q2 case.=C2=A0 Here is an example:

regression=3D# create table tgs(a int, b int);
CREATE = TABLE
regression=3D# insert into tgs values(1, 1), (1, 2), (2, 2);
IN= SERT 0 3
regression=3D# select * from (select a, b, count(*) from tgs gr= oup by grouping sets((a), (b))) t where b =3D 1;
=C2=A0a | b | count
= ---+---+-------
=C2=A0 =C2=A0| 1 | =C2=A0 =C2=A0 1
(1 row)

reg= ression=3D# select * from (select a, b, count(*) from tgs group by grouping= sets((a), (b)) =C2=A0having b =3D 1) t;
=C2=A0a | b | count
---+---+= -------
=C2=A0 =C2=A0| 1 | =C2=A0 =C2=A0 1
(1 row)

regression= =3D# select * from (select a, b, count(*) from tgs where b =3D 1 group by g= rouping sets((a), (b)) ) t;
=C2=A0a | b | count
---+---+-------
= =C2=A01 | =C2=A0 | =C2=A0 =C2=A0 1
=C2=A0 =C2=A0| 1 | =C2=A0 =C2=A0 1
(2 rows)

=C2=A0At the same time, our optimiz= er is smart enough to handle the below case (only 1 set in group sets, whic= h equals
group by).=C2=A0

regression=3D#= explain select * from (select a, b, count(*) from tgs group by grouping se= ts((a, b)) ) t where b =3D 1;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0QUERY PLAN
------= -----------------------------------------------------------
=C2=A0GroupA= ggregate =C2=A0(cost=3D38.44..38.66 rows=3D11 width=3D16)
=C2=A0 =C2=A0G= roup Key: tgs.a, tgs.b
=C2=A0 =C2=A0-> =C2=A0Sort =C2=A0(cost=3D38.44= ..38.47 rows=3D11 width=3D8)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Sort Key:= tgs.a
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0-> =C2=A0Seq Scan on tgs =C2= =A0(cost=3D0.00..38.25 rows=3D11 width=3D8)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0Filter: (b =3D 1)
(6 rows)

--
= Best Regards
Andy Fan
--00000000000007df0805ad5b7c0c--