Обсуждение: Replace use malloc() & friend by memory contexts for plperl and pltcl
Hi all, Cleanup $subject has been raised a couple of times, like one year ago here: https://www.postgresql.org/message-id/CAB7nPqRxvq+Q66UFzD9wa5UAftYN4WAUADbjXKFrync96kf-VQ@mail.gmail.com And more recently here while working on the NULL checks for malloc(): https://www.postgresql.org/message-id/CAB7nPqR7ozfCqc6C=2+ctCcnuehTZ-vTDQ8MMhY=BQyET0Honw@mail.gmail.com Attached are a set of patches to replace those memory system calls by proper memory contexts: - 0001 does the cleanup work for pltcl - 0002 does this cleanup for plperl Thoughts? -- Michael
Вложения
Michael Paquier <michael.paquier@gmail.com> writes: > Cleanup $subject has been raised a couple of times, like one year ago here: > https://www.postgresql.org/message-id/CAB7nPqRxvq+Q66UFzD9wa5UAftYN4WAUADbjXKFrync96kf-VQ@mail.gmail.com > And more recently here while working on the NULL checks for malloc(): > https://www.postgresql.org/message-id/CAB7nPqR7ozfCqc6C=2+ctCcnuehTZ-vTDQ8MMhY=BQyET0Honw@mail.gmail.com > Attached are a set of patches to replace those memory system calls by > proper memory contexts: > - 0001 does the cleanup work for pltcl > - 0002 does this cleanup for plperl I looked at 0001. It seemed to me that it wasn't that useful to add a context unless we did something about actually freeing it; otherwise we're just increasing the amount of memory leaked after a function redefinition. However, it proved pretty easy to shoehorn in a refcounting mechanism like plpgsql has, so I did that and pushed it. Off to look at 0002 next. regards, tom lane
I wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> Attached are a set of patches to replace those memory system calls by
>> proper memory contexts:
>> - 0001 does the cleanup work for pltcl
>> - 0002 does this cleanup for plperl
> Off to look at 0002 next.
Pushed 0002 as well. The main thing missing there was to use a PG_TRY
block to replace the bulky-and-yet-incomplete assorted invocations of
free_plperl_function.
regards, tom lane
On Thu, Sep 1, 2016 at 8:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> Michael Paquier <michael.paquier@gmail.com> writes: >>> Attached are a set of patches to replace those memory system calls by >>> proper memory contexts: >>> - 0001 does the cleanup work for pltcl >>> - 0002 does this cleanup for plperl > >> Off to look at 0002 next. > > Pushed 0002 as well. The main thing missing there was to use a PG_TRY > block to replace the bulky-and-yet-incomplete assorted invocations of > free_plperl_function. Thanks. That's neat! -- Michael
On 8/31/16 2:57 AM, Michael Paquier wrote: > Hi all, > > Cleanup $subject has been raised a couple of times, like one year ago here: > https://www.postgresql.org/message-id/CAB7nPqRxvq+Q66UFzD9wa5UAftYN4WAUADbjXKFrync96kf-VQ@mail.gmail.com > And more recently here while working on the NULL checks for malloc(): > https://www.postgresql.org/message-id/CAB7nPqR7ozfCqc6C=2+ctCcnuehTZ-vTDQ8MMhY=BQyET0Honw@mail.gmail.com > > Attached are a set of patches to replace those memory system calls by > proper memory contexts: > - 0001 does the cleanup work for pltcl > - 0002 does this cleanup for plperl > > Thoughts? Seems like a good idea, I'm guessing it slipped through the cracks. Do you want to add it to the next CF? I've looked at the pltcl patch, and it looks sane. I did have one question though... + volatile MemoryContext plan_cxt = NULL; ... + MemoryContext oldcontext = CurrentMemoryContext; Why mark one as volatile but not the other? Based on [1] ISTM there's no need to mark either as volatile? 1: http://www.barrgroup.com/Embedded-Systems/How-To/C-Volatile-Keyword -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On Tue, Nov 8, 2016 at 7:39 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 8/31/16 2:57 AM, Michael Paquier wrote: > Seems like a good idea, I'm guessing it slipped through the cracks. Do you > want to add it to the next CF? 0001 has been pushed as d062245b. > Why mark one as volatile but not the other? Based on [1] ISTM there's no need to mark either as volatile? plan_cxt is referenced in the PG_TRY block, and then modified in the PG_CATCH block, so it seems to me that we had better mark it as volatile to be POSIX-compliant. That's not the case of oldcontext. -- Michael