From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: xinwen@stu.scu.edu.cn, pgsql-bugs@lists.postgresql.org,
        David Rowley <dgrowleyml@gmail.com>
Subject: Re: BUG #17777: An assert failed in nodeWindowAgg.c
In-reply-to: <20230210232051.efjhtoqo6odk3gke@awork3.anarazel.de>
References: <17777-860b739b6efde977@postgresql.org>
 <20230210230230.scuk6gqrks6c7sc7@awork3.anarazel.de>
 <439391.1676070752@sss.pgh.pa.us>
 <20230210232051.efjhtoqo6odk3gke@awork3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Fri, 10 Feb 2023 15:20:51 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <443908.1676073430.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 10 Feb 2023 18:57:10 -0500
Message-ID: <443909.1676073430@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/443909.1676073430%40sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> On 2023-02-10 18:12:32 -0500, Tom Lane wrote:
>> It looks to me like maybe we could just remove the Assert and do
>> =

>> -	if (peraggstate->transValueCount =3D=3D 1)
>> +	if (peraggstate->transValueCount < 2)
>> =

>> a few lines further down?  I've not dug into the details though.

> I'd make it an ERROR, similar to this case:
> 	if (peraggstate->transValueIsNull)
> 		elog(ERROR, "aggregate transition value is NULL before inverse transit=
ion");
> afaics once we got to this point, the query results are always bogus. It=
's not
> a reliable detection of mislabeled volatility, but still seems vastly be=
tter
> than knowingly returning wrong results.

Yeah, after looking a little closer, the problem is that we may get
inconsistent results about whether a row passes the filter between
when we add the row to the trans state and when we remove from it.
If transValueCount would go negative then we know that that happened,
but it might happen without us being able to detect it.

We could force a restart, but that would only fix the results going
forward, not fix any bad values we emitted before detecting the
problem (and there almost certainly were some).

I'm content with replacing the Assert with something like

    /*
     * There should still be an added but not yet removed value; if there
     * isn't, we presumably got inconsistent results from the aggfilter
     * expression, probably due to an allegedly-immutable expression
     * delivering changing results.
     */
    if (peraggstate->transValueCount <=3D 0)
        elog(ERROR, "aggregate inverse transition failed, probably due to =
volatile FILTER expression");

I might be more excited about it if there were a visible use-case
for volatile filter expressions, but I can't see one.  The presented
test case is obviously just a fuzzer, not a useful query.

			regards, tom lane