public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tomas Vondra <[email protected]>
To: Andres Freund <[email protected]>
To: Tom Lane <[email protected]>
Cc: [email protected]
Cc: Tomas Vondra <[email protected]>
Cc: Anastasia Lubennikova <[email protected]>
Cc: Pavan Deolasee <[email protected]>
Cc: Jeff Janes <[email protected]>
Subject: Re: hio.c does visibilitymap_pin()/IO while holding buffer lock
Date: Mon, 3 Apr 2023 14:25:59 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (10+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: hio.c does visibilitymap_pin()/IO while holding buffer lock
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox