Speedup truncations of temporary relation forks

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

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

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

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

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

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]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; 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;
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

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

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

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

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

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

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

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.

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

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