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.
Attachments:
Remove-redundant-MemoryContextSwith-in-BeginCopyFrom.patchtext/x-patchDownload
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 0d6b34206a..7b3f5a84b8 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1340,10 +1340,6 @@ BeginCopyFrom(ParseState *pstate,
cstate->whereClause = whereClause;
- MemoryContextSwitchTo(oldcontext);
-
- oldcontext = MemoryContextSwitchTo(cstate->copycontext);
-
/* Initialize state variables */
cstate->eol_type = EOL_UNKNOWN;
cstate->cur_relname = RelationGetRelationName(cstate->rel);
On Wed, Jan 19, 2022 at 11:21 AM 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.
LGTM (it passed all regression without any issue)
--
Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
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.