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
Date: Fri, 10 Feb 2023 18:57:10 -0500
Message-ID: <443909.1676073430@sss.pgh.pa.us> (raw)
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>
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 == 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 transition");
> 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 better
> 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 <= 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, dgrowleyml@gmail.com
Subject: Re: BUG #17777: An assert failed in nodeWindowAgg.c
In-Reply-To: <443909.1676073430@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox