Thread: Speedup truncations of temporary relation forks
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
Attachment
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
Attachment
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