Re: Opportunistically pruning page before update
От | Melanie Plageman |
---|---|
Тема | Re: Opportunistically pruning page before update |
Дата | |
Msg-id | CAAKRu_ZZUfCaf8d1Frh7DxsvS43DRSXXjMXnycEPOzmsWp5NXQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Opportunistically pruning page before update (James Coleman <jtc331@gmail.com>) |
Список | pgsql-hackers |
On Fri, Jan 26, 2024 at 8:33 PM James Coleman <jtc331@gmail.com> wrote: > > On Tue, Jan 23, 2024 at 2:46 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Tue, Jan 23, 2024 at 7:18 AM James Coleman <jtc331@gmail.com> wrote: > > > > > > On Mon, Jan 22, 2024 at 8:21 PM James Coleman <jtc331@gmail.com> wrote: > > > > > > > > See rebased patch attached. > > > > > > I just realized I left a change in during the rebase that wasn't necessary. > > > > > > v4 attached. > > > > I have noticed that you are performing the opportunistic pruning after > > we decided that the updated tuple can not fit in the current page and > > then we are performing the pruning on the new target page. Why don't > > we first perform the pruning on the existing page of the tuple itself? > > Or this is already being done before this patch? I could not find > > such existing pruning so got this question because such pruning can > > convert many non-hot updates to the HOT update right? > > First off I noticed that I accidentally sent a different version of > the patch I'd originally worked on. Here's the one from the proper > branch. It's still similar, but I want to make sure the right one is > being reviewed. I finally got back around to looking at this. Sorry for the delay. I don't feel confident enough to say at a high level whether or not it is a good idea in the general case to try pruning every block RelationGetBufferForTuple() considers as a target block. But, I did have a few thoughts on the implementation: heap_page_prune_opt() checks PageGetHeapFreeSpace() twice. You will have already done that in RelationGetBufferForTuple(). And you don't need even need to do it both of those times because you have a lock (which is why heap_page_prune_opt() does it twice). This all seems a bit wasteful. And then, you do it again after pruning. This made me think, vacuum cares how much space heap_page_prune() (now heap_page_prune_and_freeze()) freed up. Now if we add another caller who cares how much space pruning freed up, perhaps it is worth calculating this while pruning and returning it. I know PageGetHeapFreeSpace() isn't the most expensive function in the world, but it also seems like a useful, relevant piece of information to inform the caller of. You don't have to implement the above, it was just something I was thinking about. Looking at the code also made me wonder if it is worth changing RelationGetBufferForTuple() to call PageGetHeapFreeSpace() before taking a lock (which won't give a totally accurate result, but that's probably okay) and then call heap_page_prune_opt() without a lock when PageGetHeapFreeSpace() says there isn't enough space. Also do we want to do GetVisibilityMapPins() before calling heap_page_prune_opt()? I don't quite get why we do that before knowing if we are going to actually use the target block in the current code. Anyway, I'm not sure I like just adding a parameter to heap_page_prune_opt() to indicate it already has an exclusive lock. It does a bunch of stuff that was always done without a lock and now you are doing it with an exclusive lock. - Melanie
В списке pgsql-hackers по дате отправления: