Message-ID: <96262b6442f8f752f42b50e9cc8ee69e0d9b43fc.camel@j-davis.com>
Subject: Re: pgsql: Logical Tape Set: lazily allocate read buffer.
From: Jeff Davis <pgsql@j-davis.com>
To: Tom Lane <tgl@sss.pgh.pa.us>, Jeff Davis <jdavis@postgresql.org>
Cc: pgsql-committers@lists.postgresql.org
Date: Tue, 18 Feb 2020 13:25:52 -0800
In-Reply-To: <20351.1581868306@sss.pgh.pa.us>
References: <E1j2JUY-0001ik-Cc@gemulon.postgresql.org>
	 <20351.1581868306@sss.pgh.pa.us>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun, 2020-02-16 at 10:51 -0500, Tom Lane wrote:
> 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 ==
> NULL)"?

I did that to match the original behavior, which is to only allocate
the read buffer if the tape is non-empty.

A tape with a NULL buffer is in a valid state after a rewind, though
it's precarious. It raises quite a few questions about what the valid
states of a tape are, and what usages of the API are allowed. That was
all true even before 7fdd919a (or perhaps I made a mistake and moving
the code around was not safe after all).

I think the best fix now is to just allocate the buffer even if the
tape is empty. That would stop Coverity from complaining, and I
couldn't detect any obvious performance regression.

Later, we can document the valid states a little better, and validate
them with Asserts and/or the type system.

Regards,
	Jeff Davis