From: Tom Lane <tgl@sss.pgh.pa.us>
To: Jeff Davis <pgsql@j-davis.com>
cc: =?UTF-8?Q?=E8=B5=B5=E5=BA=AD=E6=B5=B7=28=E5=BA=AD=E7=AB=A0=29?=
 <zhaotinghai.zth@alibaba-inc.com>,
        pgsql-hackers <pgsql-hackers@lists.postgresql.org>,
        Heikki Linnakangas <hlinnaka@iki.fi>
Subject: Re: sublink [exists (select xxx group by grouping sets ())] causes an
 assertion error
In-reply-to: <7242a03fe0b52e8dd24b21fc8973738105e64990.camel@j-davis.com>
References: 
 <cfd84cb8-12fe-433a-a4bb-f460a4515f9c.zhaotinghai.zth@alibaba-inc.com>
 <932664.1711124920@sss.pgh.pa.us>
 <7242a03fe0b52e8dd24b21fc8973738105e64990.camel@j-davis.com>
Comments: In-reply-to Jeff Davis <pgsql@j-davis.com>
	message dated "Fri, 22 Mar 2024 18:22:21 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <1229326.1711160246.1@sss.pgh.pa.us>
Content-Transfer-Encoding: 8bit
Date: Fri, 22 Mar 2024 22:17:26 -0400
Message-ID: <1229327.1711160246@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1229327.1711160246%40sss.pgh.pa.us>
Precedence: bulk

Jeff Davis <pgsql@j-davis.com> writes:
> On Fri, 2024-03-22 at 12:28 -0400, Tom Lane wrote:
>> Thanks for the report.  I did some bisecting and found that the crash
>> appears at Jeff's commit c8aeaf3ab (which introduced this assertion)
>> and disappears at Heikki's c4649cce3 (which removed it).  So I would
>> say that the problem is "this assertion is wrong", and we should fix
>> the problem by fixing the assertion, not by hacking around in distant
>> calling code.  On the whole, since this code has been dead for
>> several versions, I'd be inclined to just remove the assertion.

> c4649cce3 didn't add additional calls to LogicalTapeSetBlocks(), so I'm
> not sure if the removal of the Assert was related to his changes, or if
> he just realized the assertion was wrong and removed it along the way?

My guess is he just zapped it because the code block was dependent
on the "tape" abstraction which was going away.  Heikki?

> Also, without the assertion, the word "should" in the comment is
> ambiguous (does it mean "must not" or something else), and it still
> exists in master. Do we care about the calculation being wrong if
> there's an unfinished write?

I think it's actually fine.  The callers of LogicalTapeSetBlocks only
use the number for statistics or trace reporting, so precision isn't
critical in the first place, but I think what they care about is the
amount of data that's really been written out to files.  The
comment should be clarified, for sure.

			regards, tom lane