Thread: 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.
Attachment
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: > > https://commitfest.postgresql.org/47/4794/ 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)