Re: Allocations in critical section (was Re: [HACKERS] WAL format and API changes (9.5))
On 04/04/2014 04:56 PM, Tom Lane wrote: Heikki Linnakangas writes: Ok, I fixed the issues that the assertion fixed. I also committed a patch to add the assertion itself; let's see if the buildfarm finds more cases that violate the rule. It ignores the checkpointer, because it's known to violate the rule, ... uh, isn't that a bug to be fixed? Yes. One step a time ;-). and allocations in ErrorContext, which is used during error recovery, e.g if you indeed PANIC while in a critical section for some other reason. Yeah, I realized we'd have to do something about elog's own allocations. Not sure if a blanket exemption for ErrorContext is the best way. I'd been thinking of having a way to turn off the complaint once processing of an elog(PANIC) has started. Hmm. PANIC processing should avoid allocations too, except in ErrorContext, because otherwise you might get an out-of-memory during PANIC processing. ErrorContext also covers elog(DEBUG2, ...). I presume we'll want to ignore that too. Although I also tried without the exemption for ErrorContext at first, and didn't get any failures from the regression tests, so I guess we never do that in a critical section. I was a bit surprised by that. BTW, I'm pretty sure you added some redundant assertions in mcxt.c. eg, palloc does not need its own copy. palloc is copy-pasted from MemoryContextAlloc - it does need its own copy. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Allocations in critical section (was Re: [HACKERS] WAL format and API changes (9.5))
Heikki Linnakangas writes: > Ok, I fixed the issues that the assertion fixed. I also committed a > patch to add the assertion itself; let's see if the buildfarm finds more > cases that violate the rule. > It ignores the checkpointer, because it's known to violate the rule, ... uh, isn't that a bug to be fixed? > and > allocations in ErrorContext, which is used during error recovery, e.g if > you indeed PANIC while in a critical section for some other reason. Yeah, I realized we'd have to do something about elog's own allocations. Not sure if a blanket exemption for ErrorContext is the best way. I'd been thinking of having a way to turn off the complaint once processing of an elog(PANIC) has started. > I didn't backpatch this. Agreed. BTW, I'm pretty sure you added some redundant assertions in mcxt.c. eg, palloc does not need its own copy. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Allocations in critical section (was Re: [HACKERS] WAL format and API changes (9.5))
Ok, I fixed the issues that the assertion fixed. I also committed a patch to add the assertion itself; let's see if the buildfarm finds more cases that violate the rule. It ignores the checkpointer, because it's known to violate the rule, and allocations in ErrorContext, which is used during error recovery, e.g if you indeed PANIC while in a critical section for some other reason. I didn't backpatch this. Although you shouldn't be running with assertions enabled in production, it nevertheless seems too risky. There might be some obscure cases where there is no real risk, e.g because the current memory context always has enough free space because of a previous pfree, and it doesn't seem worth tracking down and fixing such issues in backbranches. You have to be pretty unlucky to run out of memory in a critical section to begin with. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
