Re: Remove redundant MemoryContextSwith in BeginCopyFrom

Re: Remove redundant MemoryContextSwith in BeginCopyFrom

Bharath Rupireddy  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

Re: Remove redundant MemoryContextSwith in BeginCopyFrom

Japin Li  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

Re: Remove redundant MemoryContextSwith in BeginCopyFrom

Re: Remove redundant MemoryContextSwith in BeginCopyFrom

On Wed, Jan 19, 2022 at 7:51 PM Japin Li  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.

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.

Re: Remove redundant MemoryContextSwith in BeginCopyFrom

On Thu, 20 Jan 2022 at 00:38, Tom Lane  wrote:
> Bharath Rupireddy  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.

Re: Remove redundant MemoryContextSwith in BeginCopyFrom

On Thu, 20 Jan 2022 at 08:50, Tom Lane  wrote:
> Japin Li  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.