Обсуждение: 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.

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)