From: Tom Lane <tgl@sss.pgh.pa.us>
To: Jeff Davis <jdavis@postgresql.org>
cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Logical Tape Set: lazily allocate read buffer.
In-reply-to: <E1j2JUY-0001ik-Cc@gemulon.postgresql.org>
References: <E1j2JUY-0001ik-Cc@gemulon.postgresql.org>
Comments: In-reply-to Jeff Davis <jdavis@postgresql.org>
	message dated "Thu, 13 Feb 2020 18:45:38 +0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <20350.1581868306.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 16 Feb 2020 10:51:46 -0500
Message-ID: <20351.1581868306@sss.pgh.pa.us>
Precedence: bulk

Jeff Davis <jdavis@postgresql.org> writes:
> Logical Tape Set: lazily allocate read buffer.

Coverity is not very pleased with this patch: it's spewing warnings like

1112         if (lt->buffer =3D=3D NULL)
1113             ltsInitReadBuffer(lts, lt);
1114     =

1115         if (blocknum !=3D lt->curBlockNumber)
1116         {
>>>     CID 1458440:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "lt->buffer" to "ltsReadBlock", which derefer=
ences it.
1117             ltsReadBlock(lts, blocknum, (void *) lt->buffer);
1118             lt->curBlockNumber =3D blocknum;

Evidently, it doesn't think that ltsInitReadBuffer() is guaranteed to
make lt->buffer not null, which is not so surprising considering
that that's coded this way:

    if (lt->firstBlockNumber !=3D -1L)
    {
        Assert(lt->buffer_size > 0);
        lt->buffer =3D palloc(lt->buffer_size);
    }

Is there a reason for that to be coded in such an obscure and fragile
fashion, rather than having the test be say "if (lt->buffer =3D=3D NULL)"?
If we really need a connection to firstBlockNumber, I'd suggest
something like

-   if (lt->firstBlockNumber !=3D -1L)
+   if (lt->buffer =3D=3D NULL)
    {
+       Assert(lt->firstBlockNumber !=3D -1L);
        Assert(lt->buffer_size > 0);
        lt->buffer =3D palloc(lt->buffer_size);
    }

but I'm not sure the extra Assert has any value.

			regards, tom lane