Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Speed up truncation of temporary relations.

  2. Speedup truncations of relation forks.

  1. Speedup truncations of temporary relation forks

    Daniil Davydov <3danissimo@gmail.com> — 2025-05-30T11:01:16Z

    Hi,
For now we fully scan local buffers for each fork of the temporary
relation that we want to truncate (in order to drop its buffers). It
happens in the function "DropRelationBuffers".
There used to be the same problem for regular tables (i.e. shared
buffers) and it was fixed in commit [1] and now shared buffers are
scanned only one time for those three relation forks.
I suggest making the same fix for temporary relations. See attached patch.

[1] 6d05086c0a79e50d8e91ed953626ec7280cd2481

BTW, I see that we call "DropRelationBuffers" separately for relation,
toast table and indexes. What if we collect all this information in
advance and iterate over the local/shared buffers only once?
I understand that it will look kinda ugly, but it will increase
performance for sure.

--
Best regards,
Daniil Davydov

  2. Re: Speedup truncations of temporary relation forks

    Michael Paquier <michael@paquier.xyz> — 2025-05-31T00:48:52Z

    On Fri, May 30, 2025 at 06:01:16PM +0700, Daniil Davydov wrote:
> For now we fully scan local buffers for each fork of the temporary
> relation that we want to truncate (in order to drop its buffers). It
> happens in the function "DropRelationBuffers".
> There used to be the same problem for regular tables (i.e. shared
> buffers) and it was fixed in commit [1] and now shared buffers are
> scanned only one time for those three relation forks.
> I suggest making the same fix for temporary relations. See attached patch.

Applying the same kind of optimization for local buffers makes sense
here, even if the impact is more limited than regular relations.

> I understand that it will look kinda ugly, but it will increase
> performance for sure.

I guess it does.  Do you have numbers to share with a test case?

Please make sure to add this patch to the next commit fest.
--
Michael

  3. Re: Speedup truncations of temporary relation forks

    Daniil Davydov <3danissimo@gmail.com> — 2025-05-31T06:26:14Z

    Hi,

On Sat, May 31, 2025 at 7:49 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, May 30, 2025 at 06:01:16PM +0700, Daniil Davydov wrote:
> > For now we fully scan local buffers for each fork of the temporary
> > relation that we want to truncate (in order to drop its buffers). It
> > happens in the function "DropRelationBuffers".
> > There used to be the same problem for regular tables (i.e. shared
> > buffers) and it was fixed in commit [1] and now shared buffers are
> > scanned only one time for those three relation forks.
> > I suggest making the same fix for temporary relations. See attached patch.
>
> Applying the same kind of optimization for local buffers makes sense
> here, even if the impact is more limited than regular relations.
>

Thanks for looking into it!

> > BTW, I see that we call "DropRelationBuffers" separately for relation,
> > toast table and indexes. What if we collect all this information in
> > advance and iterate over the local/shared buffers only once?
> > I understand that it will look kinda ugly, but it will increase
> > performance for sure.
>
> I guess it does.  Do you have numbers to share with a test case?
>

Not yet. I proceed from the assumption that if the temp_buffers
parameter is set to a large value (some users set it to more than a
gigabyte), then the vast majority of time is spent iterating through
the local buffers.
Thus, if we reduce the number of iterations from N to (for example)
N/10, we can get a 10x increase in performance. Of course, this is a
super rough assumption, but I think you understand my point.
In the near future I will prepare a patch for the idea above and try
to do some measurements. If there is a significant difference, I will
definitely let you know.

Anyway, first I suggest committing the current patch.

> Please make sure to add this patch to the next commit fest.

OK, already created.

--
Best regards,
Daniil Davydov

  4. Re: Speedup truncations of temporary relation forks

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-05-31T12:41:36Z

    
On 2025/05/31 15:26, Daniil Davydov wrote:
> Hi,
> 
> On Sat, May 31, 2025 at 7:49 AM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Fri, May 30, 2025 at 06:01:16PM +0700, Daniil Davydov wrote:
>>> For now we fully scan local buffers for each fork of the temporary
>>> relation that we want to truncate (in order to drop its buffers). It
>>> happens in the function "DropRelationBuffers".
>>> There used to be the same problem for regular tables (i.e. shared
>>> buffers) and it was fixed in commit [1] and now shared buffers are
>>> scanned only one time for those three relation forks.
>>> I suggest making the same fix for temporary relations. See attached patch.
>>
>> Applying the same kind of optimization for local buffers makes sense
>> here, even if the impact is more limited than regular relations.

+1

>> Please make sure to add this patch to the next commit fest.
> 
> OK, already created.

Here are a few review comments on the patch:

+		for (j = 0; j < nforks; j++)
  		{
-			InvalidateLocalBuffer(bufHdr, true);
+			if ((buf_state & BM_TAG_VALID) &&
+				BufTagGetForkNum(&bufHdr->tag) == forkNum[j] &&
+				bufHdr->tag.blockNum >= firstDelBlock[j])
+			{
+				InvalidateLocalBuffer(bufHdr, true);
+			}

It looks like the "buf_state & BM_TAG_VALID" check can be moved
outside the loop, along with the BufTagMatchesRelFileLocator() check.
That would avoid unnecessary looping.

Also, should we add a "break" right after calling InvalidateLocalBuffer()?
Since the buffer has already been invalidated, continuing the loop
may not be necessary.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  5. Re: Speedup truncations of temporary relation forks

    Daniil Davydov <3danissimo@gmail.com> — 2025-05-31T14:23:19Z

    Hi,

On Sat, May 31, 2025 at 7:41 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> Here are a few review comments on the patch:
>
> +               for (j = 0; j < nforks; j++)
>                 {
> -                       InvalidateLocalBuffer(bufHdr, true);
> +                       if ((buf_state & BM_TAG_VALID) &&
> +                               BufTagGetForkNum(&bufHdr->tag) == forkNum[j] &&
> +                               bufHdr->tag.blockNum >= firstDelBlock[j])
> +                       {
> +                               InvalidateLocalBuffer(bufHdr, true);
> +                       }
>
> It looks like the "buf_state & BM_TAG_VALID" check can be moved
> outside the loop, along with the BufTagMatchesRelFileLocator() check.
> That would avoid unnecessary looping.
>
> Also, should we add a "break" right after calling InvalidateLocalBuffer()?
> Since the buffer has already been invalidated, continuing the loop
> may not be necessary.

Thanks for the review! I'll fix both remarks. Please see the v2 patch.

--
Best regards,
Daniil Davydov

  6. Re: Speedup truncations of temporary relation forks

    Michael Paquier <michael@paquier.xyz> — 2025-06-01T02:22:25Z

    On Sat, May 31, 2025 at 01:26:14PM +0700, Daniil Davydov wrote:
> Not yet. I proceed from the assumption that if the temp_buffers
> parameter is set to a large value (some users set it to more than a
> gigabyte), then the vast majority of time is spent iterating through
> the local buffers.
> Thus, if we reduce the number of iterations from N to (for example)
> N/10, we can get a 10x increase in performance. Of course, this is a
> super rough assumption, but I think you understand my point.
> In the near future I will prepare a patch for the idea above and try
> to do some measurements. If there is a significant difference, I will
> definitely let you know.
> 
> Anyway, first I suggest committing the current patch.

I doubt that it would be a good idea to apply a patch "just" because
it looks like a good idea.  It is important to prove that something is
a good idea first.
--
Michael

  7. Re: Speedup truncations of temporary relation forks

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-06-01T10:30:43Z

    On Sun, Jun 1, 2025 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, May 31, 2025 at 01:26:14PM +0700, Daniil Davydov wrote:
> > Not yet. I proceed from the assumption that if the temp_buffers
> > parameter is set to a large value (some users set it to more than a
> > gigabyte), then the vast majority of time is spent iterating through
> > the local buffers.
> > Thus, if we reduce the number of iterations from N to (for example)
> > N/10, we can get a 10x increase in performance. Of course, this is a
> > super rough assumption, but I think you understand my point.
> > In the near future I will prepare a patch for the idea above and try
> > to do some measurements. If there is a significant difference, I will
> > definitely let you know.
> >
> > Anyway, first I suggest committing the current patch.
>
> I doubt that it would be a good idea to apply a patch "just" because
> it looks like a good idea.  It is important to prove that something is
> a good idea first.

I think it makes sense to do the optimization for temporary tables as
well, I tried testing with the below test case[1] and I can see ~18%
improvement with the patch.

On head it is taking ~78 ms to truncate whereas with patch it is just
taking 66ms.

[1]
set temp_buffers ='8GB';
show temp_buffers;
BEGIN;
CREATE TEMPORARY TABLE test(a int, b varchar);
INSERT INTO test select i, repeat('a', 100) from
generate_series(1,1000000) as i;
ANALYZE ;
select relpages from pg_class where relname='test';
TRUNCATE TABLE test;
ROLLBACK;

-- 
Regards,
Dilip Kumar
Google

  8. Re: Speedup truncations of temporary relation forks

    Daniil Davydov <3danissimo@gmail.com> — 2025-06-01T12:21:17Z

    Hi,

On Sun, Jun 1, 2025 at 5:31 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > I doubt that it would be a good idea to apply a patch "just" because
> > it looks like a good idea.  It is important to prove that something is
> > a good idea first.
>
> I think it makes sense to do the optimization for temporary tables as
> well, I tried testing with the below test case[1] and I can see ~18%
> improvement with the patch.
>
> On head it is taking ~78 ms to truncate whereas with patch it is just
> taking 66ms.
>
> [1]
> set temp_buffers ='8GB';
> show temp_buffers;
> BEGIN;
> CREATE TEMPORARY TABLE test(a int, b varchar);
> INSERT INTO test select i, repeat('a', 100) from
> generate_series(1,1000000) as i;
> ANALYZE ;
> select relpages from pg_class where relname='test';
> TRUNCATE TABLE test;
> ROLLBACK;

Thank you very much for your help!
I had also done some performance measurements :
set temp_buffers ='1GB';
BEGIN;
CREATE TEMP TABLE test (id INT) ON COMMIT DELETE ROWS;
INSERT INTO test SELECT generate_series(1, 30000000);
DELETE FROM test WHERE id % 10000000 = 0; -- force postgres to create fsm
ANALYZE test;
COMMIT;

*postgres was running on ramdisk with disabled swapoff*

Thus, we are creating a 1 GB table, so that the local buffers are
completely full and contain only the pages of this table.
To measure the time, I hardcoded calls of GetCurrentTimestamp and
TimestampDifference.

I got ~7% improvement with the patch. Note, that table had only 2
forks - main and fsm (I haven't figured out how to force postgres to
create a visibility map for temp table within the transaction block).

--
Best regards,
Daniil Davydov

  9. Re: Speedup truncations of temporary relation forks

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-02T03:45:59Z

    
On 2025/05/31 23:23, Daniil Davydov wrote:
> Hi,
> 
> On Sat, May 31, 2025 at 7:41 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>> Here are a few review comments on the patch:
>>
>> +               for (j = 0; j < nforks; j++)
>>                  {
>> -                       InvalidateLocalBuffer(bufHdr, true);
>> +                       if ((buf_state & BM_TAG_VALID) &&
>> +                               BufTagGetForkNum(&bufHdr->tag) == forkNum[j] &&
>> +                               bufHdr->tag.blockNum >= firstDelBlock[j])
>> +                       {
>> +                               InvalidateLocalBuffer(bufHdr, true);
>> +                       }
>>
>> It looks like the "buf_state & BM_TAG_VALID" check can be moved
>> outside the loop, along with the BufTagMatchesRelFileLocator() check.
>> That would avoid unnecessary looping.
>>
>> Also, should we add a "break" right after calling InvalidateLocalBuffer()?
>> Since the buffer has already been invalidated, continuing the loop
>> may not be necessary.
> 
> Thanks for the review! I'll fix both remarks. Please see the v2 patch.

Thanks for updating the patch! I have no further comments.

Since both you and Dilip have confirmed the performance improvement by
the patch, probably we can mark this patch as Ready for Committer?
The actual commit will need to wait until development for v19 opens, though.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  10. Re: Speedup truncations of temporary relation forks

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-06-02T04:14:20Z

    On Sun, Jun 1, 2025 at 5:51 PM Daniil Davydov <3danissimo@gmail.com> wrote:
>
> Hi,
>
> On Sun, Jun 1, 2025 at 5:31 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Sun, Jun 1, 2025 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > I doubt that it would be a good idea to apply a patch "just" because
> > > it looks like a good idea.  It is important to prove that something is
> > > a good idea first.
> >
> > I think it makes sense to do the optimization for temporary tables as
> > well, I tried testing with the below test case[1] and I can see ~18%
> > improvement with the patch.
> >
> > On head it is taking ~78 ms to truncate whereas with patch it is just
> > taking 66ms.
> >
> > [1]
> > set temp_buffers ='8GB';
> > show temp_buffers;
> > BEGIN;
> > CREATE TEMPORARY TABLE test(a int, b varchar);
> > INSERT INTO test select i, repeat('a', 100) from
> > generate_series(1,1000000) as i;
> > ANALYZE ;
> > select relpages from pg_class where relname='test';
> > TRUNCATE TABLE test;
> > ROLLBACK;
>
> Thank you very much for your help!
> I had also done some performance measurements :
> set temp_buffers ='1GB';
> BEGIN;
> CREATE TEMP TABLE test (id INT) ON COMMIT DELETE ROWS;
> INSERT INTO test SELECT generate_series(1, 30000000);
> DELETE FROM test WHERE id % 10000000 = 0; -- force postgres to create fsm
> ANALYZE test;
> COMMIT;
>
> *postgres was running on ramdisk with disabled swapoff*
>
> Thus, we are creating a 1 GB table, so that the local buffers are
> completely full and contain only the pages of this table.
> To measure the time, I hardcoded calls of GetCurrentTimestamp and
> TimestampDifference.
>
> I got ~7% improvement with the patch. Note, that table had only 2
> forks - main and fsm

+1

 (I haven't figured out how to force postgres to
> create a visibility map for temp table within the transaction block).

I haven't tested this, but I think if you do bulk copy into a table
which should mark pages all visible and after that if you delete some
tuple from pages logically it should try to update the status to not
all visible in vm?

-- 
Regards,
Dilip Kumar
Google

  11. Re: Speedup truncations of temporary relation forks

    Daniil Davydov <3danissimo@gmail.com> — 2025-06-02T06:23:41Z

    Hi,

On Mon, Jun 2, 2025 at 11:14 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> > (I haven't figured out how to force postgres to
> > create a visibility map for temp table within the transaction block).
>
> I haven't tested this, but I think if you do bulk copy into a table
> which should mark pages all visible and after that if you delete some
> tuple from pages logically it should try to update the status to not
> all visible in vm?
>

I found reliable way to create all three forks :
begin;
create temp table test (id int) on commit delete rows;
copy test from 'data.csv' with (freeze);
insert into test select generate_series(2000000, 3000000);
delete from test where id % 500000 = 0;
commit;

--
Best regards,
Daniil Davydov

  12. Re: Speedup truncations of temporary relation forks

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-06-02T09:06:34Z

    31.05.2025 17:23, Daniil Davydov пишет:
> Hi,
> 
> On Sat, May 31, 2025 at 7:41 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>> Here are a few review comments on the patch:
>>
>> +               for (j = 0; j < nforks; j++)
>>                 {
>> -                       InvalidateLocalBuffer(bufHdr, true);
>> +                       if ((buf_state & BM_TAG_VALID) &&
>> +                               BufTagGetForkNum(&bufHdr->tag) == forkNum[j] &&
>> +                               bufHdr->tag.blockNum >= firstDelBlock[j])
>> +                       {
>> +                               InvalidateLocalBuffer(bufHdr, true);
>> +                       }
>>
>> It looks like the "buf_state & BM_TAG_VALID" check can be moved
>> outside the loop, along with the BufTagMatchesRelFileLocator() check.
>> That would avoid unnecessary looping.
>>
>> Also, should we add a "break" right after calling InvalidateLocalBuffer()?
>> Since the buffer has already been invalidated, continuing the loop
>> may not be necessary.
> 
> Thanks for the review! I'll fix both remarks. Please see the v2 patch.

Excuse me for disturbing...
Wouldn't it be more efficient if we change search data structure for local
buffers?
Instead of hash table for RelFileLocator+forknum+BlockNumber it could be
hash table for RelFileLocator+forknum + included datastructure for
BlockNumber (hash table or radix tree). Then there will no be need to
iterate whole local buffers for each relation.

Given local buffers are not target for concurrent access, both hash tables
could be implemented using simplehash. It will compensate two-stage lookup,
given dynahash is much slower than simplehash.

-- 
regards
Yura Sokolov aka funny-falcon

  13. Re: Speedup truncations of temporary relation forks

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-04T13:18:36Z

    
On 2025/06/02 18:06, Yura Sokolov wrote:
> 31.05.2025 17:23, Daniil Davydov пишет:
>> Hi,
>>
>> On Sat, May 31, 2025 at 7:41 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>
>>> Here are a few review comments on the patch:
>>>
>>> +               for (j = 0; j < nforks; j++)
>>>                  {
>>> -                       InvalidateLocalBuffer(bufHdr, true);
>>> +                       if ((buf_state & BM_TAG_VALID) &&
>>> +                               BufTagGetForkNum(&bufHdr->tag) == forkNum[j] &&
>>> +                               bufHdr->tag.blockNum >= firstDelBlock[j])
>>> +                       {
>>> +                               InvalidateLocalBuffer(bufHdr, true);
>>> +                       }
>>>
>>> It looks like the "buf_state & BM_TAG_VALID" check can be moved
>>> outside the loop, along with the BufTagMatchesRelFileLocator() check.
>>> That would avoid unnecessary looping.
>>>
>>> Also, should we add a "break" right after calling InvalidateLocalBuffer()?
>>> Since the buffer has already been invalidated, continuing the loop
>>> may not be necessary.
>>
>> Thanks for the review! I'll fix both remarks. Please see the v2 patch.
> 
> Excuse me for disturbing...
> Wouldn't it be more efficient if we change search data structure for local
> buffers?
> Instead of hash table for RelFileLocator+forknum+BlockNumber it could be
> hash table for RelFileLocator+forknum + included datastructure for
> BlockNumber (hash table or radix tree). Then there will no be need to
> iterate whole local buffers for each relation.
> 
> Given local buffers are not target for concurrent access, both hash tables
> could be implemented using simplehash. It will compensate two-stage lookup,
> given dynahash is much slower than simplehash.

I'm not sure how much this approach improves performance, but it might
be worth trying. If it proves effective, it would also make sense to
apply it to shared buffers, since it's typically larger and takes longer
to scan than local buffers.

Regardless, I think we should go ahead and apply the current patch.
If your approach shows a noticeable performance gain, we can consider
adding it as a follow-up.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  14. Re: Speedup truncations of temporary relation forks

    Maxim Orlov <orlovmg@gmail.com> — 2025-06-04T14:43:37Z

    On Wed, 4 Jun 2025 at 16:18, Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

>
> Regardless, I think we should go ahead and apply the current patch.
>

Yeah, it is definitely improving things. It turns out that I made almost
the same
patch for our fork of Postgres, commissioned by one of the clients dealing
with
truncation of huge amount of temp relations. Back in the days, it solves
the
problem for him. But then I utterly forgot to share it with the community.
Shame on me.

Anyway, the patch looks good to me. Hope, it will be committed soon.

-- 
Best regards,
Maxim Orlov.

  15. Re: Speedup truncations of temporary relation forks

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-04T00:10:31Z

    
On 2025/06/04 23:43, Maxim Orlov wrote:
> 
> On Wed, 4 Jun 2025 at 16:18, Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote:
> 
> 
>     Regardless, I think we should go ahead and apply the current patch.
> 
> 
> Yeah, it is definitely improving things. It turns out that I made almost the same
> patch for our fork of Postgres, commissioned by one of the clients dealing with
> truncation of huge amount of temp relations. Back in the days, it solves the
> problem for him. But then I utterly forgot to share it with the community.
> Shame on me.
> 
> Anyway, the patch looks good to me. Hope, it will be committed soon.

Patch pushed. Thanks, everyone!

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  16. Re: Speedup truncations of temporary relation forks

    Daniil Davydov <3danissimo@gmail.com> — 2025-07-04T03:13:03Z

    Hi,

On Fri, Jul 4, 2025 at 7:10 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> Patch pushed. Thanks, everyone!
>

Glad to hear this. Thanks!

--
Best regards,
Daniil Davydov
Postgres Professional