Обсуждение: Remove redundant MemoryContextSwith in BeginCopyFrom

Hi, hackers

When I read the code of COPY ... FROM ..., I find there is a redundant
MemoryContextSwith() in BeginCopyFrom().  In BeginCopyFrom, it creates
a COPY memory context and then switches to it, in the middle of this
function, it switches to the oldcontext and immediately switches back to
COPY memory context, IMO, this is redundant, and can be removed safely.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Wed, Jan 19, 2022 at 7:51 PM Japin Li <japinli@hotmail.com> wrote:
>
>
> Hi, hackers
>
> When I read the code of COPY ... FROM ..., I find there is a redundant
> MemoryContextSwith() in BeginCopyFrom().  In BeginCopyFrom, it creates
> a COPY memory context and then switches to it, in the middle of this
> function, it switches to the oldcontext and immediately switches back to
> COPY memory context, IMO, this is redundant, and can be removed safely.

+1. It looks like a thinko from c532d15d. There's no code in between,
so switching to oldcontext doesn't make sense.

    MemoryContextSwitchTo(oldcontext);
    << no code here >>
    oldcontext = MemoryContextSwitchTo(cstate->copycontext);

I think we also need to remove MemoryContextSwitchTo(oldcontext); at
the end of BeginCopyTo in copyto.c, because we are not changing memory
contexts in between.

diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 34c8b80593..5182048e4f 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -742,8 +742,6 @@ BeginCopyTo(ParseState *pstate,

        cstate->bytes_processed = 0;

-       MemoryContextSwitchTo(oldcontext);
-
        return cstate;
 }

Regards,
Bharath Rupireddy.

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> +1. It looks like a thinko from c532d15d. There's no code in between,
> so switching to oldcontext doesn't make sense.

Agreed.

> I think we also need to remove MemoryContextSwitchTo(oldcontext); at
> the end of BeginCopyTo in copyto.c, because we are not changing memory
> contexts in between.

Hmm, I think it'd be a better idea to remove the one in the middle of
BeginCopyTo.  The code after that is still doing setup of the cstate,
so the early switch back looks to me like trouble waiting to happen.

            regards, tom lane

On Thu, 20 Jan 2022 at 00:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
>> +1. It looks like a thinko from c532d15d. There's no code in between,
>> so switching to oldcontext doesn't make sense.
>
> Agreed.
>
>> I think we also need to remove MemoryContextSwitchTo(oldcontext); at
>> the end of BeginCopyTo in copyto.c, because we are not changing memory
>> contexts in between.
>
> Hmm, I think it'd be a better idea to remove the one in the middle of
> BeginCopyTo.  The code after that is still doing setup of the cstate,
> so the early switch back looks to me like trouble waiting to happen.
>

Agreed

I see you have already push this patch on master (89f059bdf52), why not
remove MemoryContextSwitchTo in the middle of BeginCopyTo in this commit?

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:
> I see you have already push this patch on master (89f059bdf52), why not
> remove MemoryContextSwitchTo in the middle of BeginCopyTo in this commit?

That was a different suggestion from a different person, so I didn't
want to muddle the credit.  Also, it requires a bit of testing,
while 89f059bdf52 was visibly perfectly safe.

            regards, tom lane

On Thu, 20 Jan 2022 at 08:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Japin Li <japinli@hotmail.com> writes:
>> I see you have already push this patch on master (89f059bdf52), why not
>> remove MemoryContextSwitchTo in the middle of BeginCopyTo in this commit?
>
> That was a different suggestion from a different person, so I didn't
> want to muddle the credit.  Also, it requires a bit of testing,
> while 89f059bdf52 was visibly perfectly safe.
>

Thanks for your explanation!

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.