Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove redundant memory context switches in BeginCopyFrom().

  1. Remove redundant MemoryContextSwith in BeginCopyFrom

    Japin Li <japinli@hotmail.com> — 2022-01-19T14:20:58Z

    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.

  2. Re: Remove redundant MemoryContextSwith in BeginCopyFrom

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2022-01-19T15:34:32Z

    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

  3. Re: Remove redundant MemoryContextSwith in BeginCopyFrom

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-01-19T15:35:09Z

    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.

  4. Re: Remove redundant MemoryContextSwith in BeginCopyFrom

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-19T16:38:44Z

    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

  5. Re: Remove redundant MemoryContextSwith in BeginCopyFrom

    Japin Li <japinli@hotmail.com> — 2022-01-20T00:44:17Z

    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.

  6. Re: Remove redundant MemoryContextSwith in BeginCopyFrom

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-20T00:50:31Z

    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

  7. Re: Remove redundant MemoryContextSwith in BeginCopyFrom

    Japin Li <japinli@hotmail.com> — 2022-01-20T00:55:45Z

    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.