MIME-Version: 1.0
References: <98e18ffc-c402-4710-ae28-6d6dbcf08e44@altlinux.org>
 <FDD95AB2-17BF-4C52-A8A9-682115FB4684@yesql.se>
 <ba1926b5-74c6-42c8-8f65-f3de9cbac026@altlinux.org>
 <18bda8da-1ab0-4a4d-9aca-826610a244a8@altlinux.org>
 <CAOYmi+k9k8U_K=zE4067iRs=01r3XA8uBUr72LkcrjEYMzUO5A@mail.gmail.com>
In-Reply-To: 
 <CAOYmi+k9k8U_K=zE4067iRs=01r3XA8uBUr72LkcrjEYMzUO5A@mail.gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Tue, 22 Jul 2025 14:25:27 +1200
Message-ID: 
 <CAApHDvo2N2SafdpgNEhny+GSq-AJLv=_MugFx0L0xAz1x+8dMg@mail.gmail.com>
Subject: Re: [PATCH] Check for TupleTableSlot nullness before dereferencing
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: kuznetsovam@altlinux.org, Daniel Gustafsson <daniel@yesql.se>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
 egori@altlinux.org,
	nickel@altlinux.org, Nikolay Shaplov <dhyan@nataraj.su>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvo2N2SafdpgNEhny%2BGSq-AJLv%3D_MugFx0L0xAz1x%2B8dMg%40mail.gmail.com>
Precedence: bulk

On Tue, 22 Jul 2025 at 10:20, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Fri, Dec 13, 2024 at 12:54=E2=80=AFAM Alexander Kuznetsov
> > 1. slot2 is NULL at line 968,
> > 2. The while loop at line 971 executes once, filling slot1 (slot2 still=
 remains NULL),
> > 3. No changes occur to slot2 thereafter, up to line 1003,
> > 4. At line 1003, slot2 is swapped with slot1 (assuming numDistinctCols =
> 0),
> > 5. At line 1016, slot1 is dereferenced with conent of slot2 (NULL).
>
> assumes that (numDistinctCols > 0) can be true at the same time that
> (slot2 =3D=3D NULL), but it's not clear to me that this can happen in
> practice. See build_pertrans_for_aggref(), where these are assigned.

I also can't see how this could happen. uniqslot is created in
build_pertrans_for_aggref() when "numDistinctCols > 0", and the swap
of slot1 and slot2 also only occurs when "numDistinctCols > 0". i.e.
the uniqslot will always be allocated when the aggregate has DISTINCT
(unless it's a presorted aggregate, in which case
process_ordered_aggregate_multi() isn't used).

If numDistinctCols > 0 then so is numSortCols > 0 per "numSortCols =3D
numDistinctCols =3D list_length(sortlist);" in
build_pertrans_for_aggref().

The code in this area gets a bit of exercise with:

create table ab(a text, b text);
insert into ab values('1','1'),('1','1');
set enable_presorted_aggregate=3D0;
select string_agg(distinct a,b) from ab;

David