Re: hio.c does visibilitymap_pin()/IO while holding buffer lock

On 4/3/23 00:40, Andres Freund wrote:
> Hi,
> 
> On 2023-03-28 19:17:21 -0700, Andres Freund wrote:
>> On 2023-03-28 18:21:02 -0700, Andres Freund wrote:
>>> Here's a draft patch.
>>
>> Attached is v2, with a stupid bug fixed and a bit of comment / pgindent
>> polish.
> 
> I'd welcome some review (Tomas?), but otherwise I'm planning to push ahead
> with this.
> 

I guess the 0001 part was already pushed, so I should be looking only at
0002, correct?

I think 0002 makes RelationGetBufferForTuple() harder to understand. I'm
not saying it's incorrect, but I find it hard to reason about the new
combinations of conditions :-(

I mean, it only had this condition:

    if (otherBuffer != InvalidBuffer)
    {
        ...
    }

but now it have

    if (unlockedTargetBuffer)
    {
       ...
    }
    else if (otherBuffer != InvalidBuffer)
    {
        ...
    }

    if (unlockedTargetBuffer || otherBuffer != InvalidBuffer)
    {
        ...
    }

Not sure how to improve that :-/ but not exactly trivial to figure out
what's going to happen.

Maybe this

 * If we unlocked the target buffer above, it's unlikely, but possible,
 * that another backend used space on this page.

might say what we're going to do in this case. I mean - I understand
some backend may use space in unlocked page, but what does that mean for
this code? What is it going to do? (The same comment talks about the
next condition in much more detail, for example.)

Also, isn't it a bit strange the free space check now happens outside
any if condition? It used to happen in the

    if (otherBuffer != InvalidBuffer)
    {
        ...
    }

block, but now it happens outside.

> I'm still debating with myself whether this commit (or a prerequisite commit)
> should move logic dealing with the buffer ordering into
> GetVisibilityMapPins(), so we don't need two blocks like this:
> 
> 
>         if (otherBuffer == InvalidBuffer || targetBlock <= otherBlock)
>             GetVisibilityMapPins(relation, buffer, otherBuffer,
>                                  targetBlock, otherBlock, vmbuffer,
>                                  vmbuffer_other);
>         else
>             GetVisibilityMapPins(relation, otherBuffer, buffer,
>                                  otherBlock, targetBlock, vmbuffer_other,
>                                  vmbuffer);
> ...
> 
>         if (otherBuffer != InvalidBuffer)
>         {
>             if (GetVisibilityMapPins(relation, otherBuffer, buffer,
>                                      otherBlock, targetBlock, vmbuffer_other,
>                                      vmbuffer))
>                 unlockedTargetBuffer = true;
>         }
>         else
>         {
>             if (GetVisibilityMapPins(relation, buffer, InvalidBuffer,
>                                      targetBlock, InvalidBlockNumber,
>                                      vmbuffer, InvalidBuffer))
>                 unlockedTargetBuffer = true;
>         }
>     }
> 

Yeah. I haven't tried, but I imagine it'd make RelationGetBufferForTuple
a little bit.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company