BeginCopyTo - remove switching to old memory context in between COPY TO command processing

Started by Bharath Rupireddyalmost 4 years ago3 messages

bharath.rupireddyforpostgres@gmail.com

almost 4 years ago

1 attachment(s)

Hi,

While reviewing patch at [1]/messages/by-id/539760.1642610324@sss.pgh.pa.us, it has been found that the memory
context switch to oldcontext from cstate->copycontext in between
BeginCopyTo is not correct because the intention of the copycontext is
to use it through the copy command processing. It looks like a thinko
from the commit c532d1 [2]commit c532d15dddff14b01fe9ef1d465013cb8ef186df Author: Heikki Linnakangas <heikki.linnakangas@iki.fi> Date: Mon Nov 23 10:50:50 2020 +0200. Attaching a small patch to remove this.

Thoughts?

[1]: /messages/by-id/539760.1642610324@sss.pgh.pa.us
[2]: commit c532d15dddff14b01fe9ef1d465013cb8ef186df Author: Heikki Linnakangas <heikki.linnakangas@iki.fi> Date: Mon Nov 23 10:50:50 2020 +0200
commit c532d15dddff14b01fe9ef1d465013cb8ef186df
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon Nov 23 10:50:50 2020 +0200

Split copy.c into four files.

Regards,
Bharath Rupireddy.

Attachments:

v1-0001-remove-switching-to-old-memory-context-in-between.patchapplication/octet-stream; name=v1-0001-remove-switching-to-old-memory-context-in-between.patchDownload

From 048203ab202241afa3d6d95445333a3c89af62dd Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Fri, 28 Jan 2022 09:46:14 +0000
Subject: [PATCH v1] remove switching to old memory context in between

---
 src/backend/commands/copyto.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 20bfd49112..2ca5bbbaff 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -657,8 +657,6 @@ BeginCopyTo(ParseState *pstate,
 
 	cstate->copy_dest = COPY_FILE;	/* default */
 
-	MemoryContextSwitchTo(oldcontext);
-
 	if (pipe)
 	{
 		progress_vals[1] = PROGRESS_COPY_TYPE_PIPE;
-- 
2.25.1

Japin Li

japinli@hotmail.com

almost 4 years ago

In reply to: Bharath Rupireddy (#1)

Re: BeginCopyTo - remove switching to old memory context in between COPY TO command processing

On Fri, 28 Jan 2022 at 18:11, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

While reviewing patch at [1], it has been found that the memory
context switch to oldcontext from cstate->copycontext in between
BeginCopyTo is not correct because the intention of the copycontext is
to use it through the copy command processing. It looks like a thinko
from the commit c532d1 [2]. Attaching a small patch to remove this.

Thoughts?

Thanks for the patch! Tested and passed regression tests. LGTM.

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

Michael Paquier

michael@paquier.xyz

almost 4 years ago

In reply to: Bharath Rupireddy (#1)

Re: BeginCopyTo - remove switching to old memory context in between COPY TO command processing

On Fri, Jan 28, 2022 at 03:41:11PM +0530, Bharath Rupireddy wrote:

While reviewing patch at [1], it has been found that the memory
context switch to oldcontext from cstate->copycontext in between
BeginCopyTo is not correct because the intention of the copycontext is
to use it through the copy command processing. It looks like a thinko
from the commit c532d1 [2]. Attaching a small patch to remove this.

Thoughts?

I think that you are right. Before c532d15d, BeginCopy() was
internally doing one switch with copycontext and the current memory
context. This commit has just moved the internals of BeginCopy()
into BeginCopyTo(), so this looks like a copy-paste error to me. I'll
go fix it, thanks for the report!
-
Michael