Date: Wed, 19 Apr 2023 11:17:04 -0400
From: Melanie Plageman <melanieplageman@gmail.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
	Richard Guo <guofenglinux@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Wrong results from Parallel Hash Full Join
Message-ID: <20230419151704.xkkobswffutslj34@liskov>
Mail-Followup-To: Thomas Munro <thomas.munro@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Richard Guo <guofenglinux@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <CAMbWs48Nde1Mv=BJv6_vXmRKHMuHZm2Q_g4F6Z3_pn+3EV6BGQ@mail.gmail.com>
 <CAAKRu_aaaGtjJK5Jt2NKbs-X2v9md-sW8em7kzMHpwZstrLT-w@mail.gmail.com>
 <20230412181452.cyfduw2ka2atqmii@awork3.anarazel.de>
 <CAAKRu_Z=rzXjxVtA7gPe2bmBk7F6e4_pe8iT5DmnYFOgWPxMGQ@mail.gmail.com>
 <CAAKRu_Ybw_0MDNTW_jg3gndXs7F6H8MUZkbY2iMtSHeS5L97hw@mail.gmail.com>
 <CA+hUKGKuLORE0OQ20sFrfdp=5tKVvamB-FnnCS03UZE+o8UFjQ@mail.gmail.com>
 <CAAKRu_av+NCVPXkLLTBG2VZcAk83FrzR9z-kZ-NQ+1kai9Tibw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAAKRu_av+NCVPXkLLTBG2VZcAk83FrzR9z-kZ-NQ+1kai9Tibw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230419151704.xkkobswffutslj34%40liskov>
Precedence: bulk

On Wed, Apr 12, 2023 at 08:31:26PM -0400, Melanie Plageman wrote:
> On Wed, Apr 12, 2023 at 6:50 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > And if we're going to
> > exercise/test that case, should we do the non-parallel version too?
> 
> I've added this. I thought if we were adding the serial case, we might
> as well add the multi-batch case as well. However, that proved a bit
> more challenging. We can get a HOT tuple in one of the existing tables
> with no issues. Doing this and then deleting the reset match bit code
> doesn't cause any of the tests to fail, however, because we use this
> expression as the join condition when we want to emit NULL-extended
> unmatched tuples.
> 
> select  count(*) from simple r full outer join simple s on (r.id = 0 - s.id);
> 
> I don't think we want to add yet another time-consuming test to this
> test file. So, I was trying to decide if it was worth changing these
> existing tests so that they would fail when the match bit wasn't reset.
> I'm not sure.

I couldn't stop thinking about how my explanation for why this test
didn't fail sounded wrong.

After some further investigation, I found that the real reason that the
HOT bit is already cleared in the tuples inserted into the hashtable for
this query is that the tuple descriptor for the relation "simple" and
the target list for the scan node are not identical (because we only
need to retain a single column from simple in order to eventually do
count(*)), so we make a new virtual tuple and build projection info for
the scan node. The virtual tuple doesn't have the HOT bit set anymore
(the buffer heap tuple would have). So we couldn't fail a test of the
code clearing the match bit.

Ultimately this is probably fine. If we wanted to modify one of the
existing tests to cover the multi-batch case, changing the select
count(*) to a select * would do the trick. I imagine we wouldn't want to
do this because of the excessive output this would produce. I wondered
if there was a pattern in the tests for getting around this. But,
perhaps we don't care enough to cover this code.

- Melanie