MIME-Version: 1.0
References: <FCB61654-575D-4F08-AA7E-ED462EDE48A7@gmail.com>
 <SY7PR01MB1092127415D38E676B1B0BAA0B648A@SY7PR01MB10921.ausprd01.prod.outlook.com>
In-Reply-To: 
 <SY7PR01MB1092127415D38E676B1B0BAA0B648A@SY7PR01MB10921.ausprd01.prod.outlook.com>
From: Xuneng Zhou <xunengzhou@gmail.com>
Date: Tue, 31 Mar 2026 09:09:47 +0800
Message-ID: 
 <CABPTF7XJ3gv+R+4+eGMX=ggN1m6bD7=rbxDi-cHN+GJUV5jSYg@mail.gmail.com>
Subject: Re: heapam_tuple_complete_speculative : remove unnecessary tuple
 fetch
To: Japin Li <japinli@hotmail.com>
Cc: Chao Li <li.evan.chao@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABPTF7XJ3gv%2BR%2B4%2BeGMX%3DggN1m6bD7%3DrbxDi-cHN%2BGJUV5jSYg%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 24, 2026 at 8:16=E2=80=AFPM Japin Li <japinli@hotmail.com> wrot=
e:
>
>
> > =E5=9C=A8 2026=E5=B9=B43=E6=9C=8824=E6=97=A5=EF=BC=8C14:57=EF=BC=8CChao=
 Li <li.evan.chao@gmail.com> =E5=86=99=E9=81=93=EF=BC=9A
> >
> > =EF=BB=BFHi,
> >
> > While reviewing another patch, I noticed this:
> > ```
> > static void
> > heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *sl=
ot,
> >                                  uint32 specToken, bool succeeded)
> > {
> >    bool        shouldFree =3D true;
> >    HeapTuple    tuple =3D ExecFetchSlotHeapTuple(slot, true, &shouldFre=
e); // <=3D=3D tuple is not used
> >
> >    /* adjust the tuple's state accordingly */
> >    if (succeeded)
> >        heap_finish_speculative(relation, &slot->tts_tid);
> >    else
> >        heap_abort_speculative(relation, &slot->tts_tid);
> >
> >    if (shouldFree)
> >        pfree(tuple);
> > }
> > ```
> >
> > In this function, tuple is not used at all, so there seems to be no nee=
d to fetch it, and shouldFree is thus not needed either.
> >
> > This appears to have been there since 5db6df0c011, where the function w=
as introduced. It looks like a copy-pasto from the previous function, heapa=
m_tuple_insert_speculative(), which does need to fetch and possibly free th=
e tuple.
> >
> > I tried simply removing ExecFetchSlotHeapTuple(), and "make check" stil=
l passes. But I may be missing something, so I=E2=80=99d like to confirm.
> >
> > The attached patch just removes the unused tuple and shouldFree from th=
is function. As touching the file, I also fixed a typo in the file header c=
omment.
>
> Makes sense! All test cases passed with make check-world.
>

+1, looks like a simple copy-pasto and the patch LGTM.

--=20
Best,
Xuneng