From: Tom Lane <tgl@sss.pgh.pa.us>
To: dmigowski@ikoffice.de
cc: pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
Subject: Re: BUG #18205: Performance regression with NOT NULL checks.
In-reply-to: <18205-53c59cab45881d52@postgresql.org>
References: <18205-53c59cab45881d52@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Sun, 19 Nov 2023 15:26:32 +0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1130863.1700420885.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 19 Nov 2023 14:08:05 -0500
Message-ID: <1130864.1700420885@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1130864.1700420885%40sss.pgh.pa.us>
Precedence: bulk

PG Bug reporting form <noreply@postgresql.org> writes:
> I found out that selecting from a wide table with a single not-null wher=
e
> clause leads to severe performance regression when upgrading from Postgr=
eSQL
> 9.5 to PostgreSQL 15.

I spent some time poking into this.  "git bisect" pins the blame on

commit b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755
Author: Andres Freund <andres@anarazel.de>
Date:   Tue Mar 14 15:45:36 2017 -0700

    Faster expression evaluation and targetlist projection.

The time needed for the seqscan gets about 50% worse at that
commit (on my test machine, anyway):

   ->  Seq Scan on testtable t  (cost=3D0.00..51892.80 rows=3D9730 width=3D=
4) (actual time=3D0.010..204.937 rows=3D6000000 loops=3D1)
         Filter: (y IS NULL)

versus

   ->  Seq Scan on testtable t  (cost=3D0.00..51892.80 rows=3D9730 width=3D=
4) (actual time=3D0.013..317.069 rows=3D6000000 loops=3D1)
         Filter: (y IS NULL)

"perf" says that the extra time is mostly spent in slot_deform_tuple's
inner loop:

    for (; attnum < natts; attnum++)
    {
        Form_pg_attribute thisatt =3D att[attnum];

        if (hasnulls && att_isnull(attnum, bp))
        {
            values[attnum] =3D (Datum) 0;
            isnull[attnum] =3D true;
            slow =3D true;        /* can't use attcacheoff anymore */
            continue;
        }
        ...

which confused me, because that code doesn't look materially different
in v10 than 9.6.  I eventually realized that the reason is that we
reach slot_deform_tuple with natts =3D 26 in the new code, but in the
old code we do not, thanks to this short-circuit in slot_getattr:

    /*
     * check if target attribute is null: no point in groveling through tu=
ple
     */
    if (HeapTupleHasNulls(tuple) && att_isnull(attnum - 1, tup->t_bits))
    {
        *isnull =3D true;
        return (Datum) 0;
    }

So that results in not having to deconstruct most of the tuple,
whereas in the new code we do have to, thanks to b8d7f053c's
decision to batch all the variable-value-extraction work.

This is a pretty narrow corner case: it would only matter if the
column you're testing for null-ness is far past any other column
the query needs to fetch.  So I'm not sure that it's worth doing
anything about.  You could imagine special processing for
NullTest-on-a-simple-Var: exclude the Var from the ones that we
extract normally and instead compile it into some sort of "is column k
NULL" test on the HeapTuple.  But that seems messy, and it could be a
significant pessimization for storage methods that aren't like heapam.

On the whole I'm inclined to say "sorry, that's the price of
progress".  But it is a bit sad that a patch intended to be a
performance win shows a loss this big on a (presumably) real-world
case.

			regards, tom lane