Re: BUG #18259: Assertion in ExtendBufferedRelLocal() fails after no-space-left condition

Hi,

On 2023-12-28 14:36:46 +0800, Richard Guo wrote:
> On Wed, Dec 27, 2023 at 5:08 PM tender wang <tndrwang@gmail.com> wrote:
> 
> > Alexander Lakhin <exclusion@gmail.com> 于2023年12月27日周三 15:00写道：
> >
> >> I wonder, if "buf_state &= BM_VALID" is a typo here, maybe it supposed to
> >> be
> >> "buf_state &= ~BM_VALID" as in ExtendBufferedRelShared()...
> >>
> >
> > Yeah, that's true.  I analyze this issue again, and I think the root cause
> > is the " buf_state &= BM_VALID" .
> >
> 
> Nice catch.

Indeed.

I wonder how this survived - I remember having written a script to locally
test this scenario...

Hm, I dimly remember having a hard time making my approach for this work for
temp tables - IIRC used chattr +i to makr the file immutable, which only works
for file descriptors opened after.


> I believe the intention is to clear the BM_VALID bit.

Indeed.


These paths had many bugs over the years, they are hard to hit in testing but
regularly hit in production.  In a more modern world we'd have a unit test for
these scenarios, in isolation. But that's hard at the moment.

I have been wondering if it's worth and possible to write a C level test for
them, perhaps in regress.c. But right now that seems hard because of the
DropRelationBuffers() in smgrtruncate() (seems like a layering violation to
have it there, but it's probably too risky to move at this point).

But perhaps this could actually serve as a good first case for Michael's
failure injection patch?  A failure injection to make
FileFallocate()/FileZero() fail should just be a few lines.

We generally have pretty much no coverage for out-of-space, partial
read/write, EINTR of file operations, because that's hard to do with the
current test infrastructure. So this might be a nice first case.

Greetings,

Andres Freund