Unnecessary smgropen in {heapam_relation,index}_copy_data?
Hi, hackers
I find heapam_relation_copy_data() and index_copy_data() have the following code:
dstrel = smgropen(*newrlocator, rel->rd_backend);
...
RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
The smgropen() is also called by RelationCreateStorage(), why should we call
smgropen() explicitly here?
I try to remove the smgropen(), and all tests passed.
Attachments:
v1-0001-Remove-unnecessary-smgropen.patchtext/x-diffDownload
From 88a6670dcff67036014fd6b31062bcab5daed30e Mon Sep 17 00:00:00 2001
From: japinli <japinli@hotmail.com>
Date: Tue, 23 Jan 2024 12:34:18 +0800
Subject: [PATCH v1 1/1] Remove unnecessary smgropen
---
src/backend/access/heap/heapam_handler.c | 4 +---
src/backend/commands/tablecmds.c | 4 +---
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 5a17112c91..547fdef26a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -637,8 +637,6 @@ heapam_relation_copy_data(Relation rel, const RelFileLocator *newrlocator)
{
SMgrRelation dstrel;
- dstrel = smgropen(*newrlocator, rel->rd_backend);
-
/*
* Since we copy the file directly without looking at the shared buffers,
* we'd better first flush out any pages of the source relation that are
@@ -654,7 +652,7 @@ heapam_relation_copy_data(Relation rel, const RelFileLocator *newrlocator)
* NOTE: any conflict in relfilenumber value will be caught in
* RelationCreateStorage().
*/
- RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
+ dstrel = RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
/* copy main fork */
RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index da705ae468..5ca6277ee0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14787,8 +14787,6 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
{
SMgrRelation dstrel;
- dstrel = smgropen(newrlocator, rel->rd_backend);
-
/*
* Since we copy the file directly without looking at the shared buffers,
* we'd better first flush out any pages of the source relation that are
@@ -14804,7 +14802,7 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
* NOTE: any conflict in relfilenumber value will be caught in
* RelationCreateStorage().
*/
- RelationCreateStorage(newrlocator, rel->rd_rel->relpersistence, true);
+ dstrel = RelationCreateStorage(newrlocator, rel->rd_rel->relpersistence, true);
/* copy main fork */
RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM,
base-commit: 80bece312c4b957ea5a93db84be1d1776f0e5e67
--
2.34.1
Hi,
I find heapam_relation_copy_data() and index_copy_data() have the following code:
dstrel = smgropen(*newrlocator, rel->rd_backend);
...
RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
The smgropen() is also called by RelationCreateStorage(), why should we call
smgropen() explicitly here?I try to remove the smgropen(), and all tests passed.
That's a very good question. Note that the second argument of
smgropen() used to create dstrel changes after applying your patch.
I'm not 100% sure whether this is significant or not.
I added your patch to the nearest open commitfest so that we will not lose it:
https://commitfest.postgresql.org/47/4794/
--
Best regards,
Aleksander Alekseev
On Thu, 25 Jan 2024 at 21:43, Aleksander Alekseev <aleksander@timescale.com> wrote:
Hi,
I find heapam_relation_copy_data() and index_copy_data() have the following code:
dstrel = smgropen(*newrlocator, rel->rd_backend);
...
RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
The smgropen() is also called by RelationCreateStorage(), why should we call
smgropen() explicitly here?I try to remove the smgropen(), and all tests passed.
That's a very good question. Note that the second argument of
smgropen() used to create dstrel changes after applying your patch.
I'm not 100% sure whether this is significant or not.
Thanks for the review.
According the comments of RelationData->rd_backend, it is the backend id, if
the relation is temporary. The differnece is RelationCreateStorage() uses
relpersistence to determinate the backend id.
I added your patch to the nearest open commitfest so that we will not lose it:
Thank you.
On 25/01/2024 17:22, Japin Li wrote:
On Thu, 25 Jan 2024 at 21:43, Aleksander Alekseev <aleksander@timescale.com> wrote:
I find heapam_relation_copy_data() and index_copy_data() have the following code:
dstrel = smgropen(*newrlocator, rel->rd_backend);
...
RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
The smgropen() is also called by RelationCreateStorage(), why should we call
smgropen() explicitly here?I try to remove the smgropen(), and all tests passed.
That's a very good question. Note that the second argument of
smgropen() used to create dstrel changes after applying your patch.
I'm not 100% sure whether this is significant or not.Thanks for the review.
According the comments of RelationData->rd_backend, it is the backend id, if
the relation is temporary. The differnece is RelationCreateStorage() uses
relpersistence to determinate the backend id.
Committed, thanks!
--
Heikki Linnakangas
Neon (https://neon.tech)