Re: WAL format and API changes (9.5)

On 04/04/2014 02:40 AM, Andres Freund wrote:
> On 2014-04-03 19:33:12 -0400, Tom Lane wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>> On 2014-04-03 19:08:27 -0400, Tom Lane wrote:
>>>> A somewhat more relevant concern is where are we going to keep the state
>>>> data involved in all this. Since this code is, by definition, going to be
>>>> called in critical sections, any solution involving internal pallocs is
>>>> right out.
>>
>>> We actually already allocate memory in XLogInsert() :(, although only in
>>> the first XLogInsert() a backend does...
>>
>> Ouch. I wonder if we should put an Assert(not-in-critical-section)
>> into MemoryContextAlloc.

Good idea!

> XLogInsert() is using malloc() directly, so that wouldn't detect this
> case...
>
> It's not a bad idea tho. I wonder how far the regression tests
> get...
>
> Not even through initdb. GetVirtualXIDsDelayingChkpt() is to blame.

Hmm, the checkpointer process seems to ignore the rule, and just hopes
for the best. The allocation in GetVirtualXIDsDelayingChkpt() was
probably just an oversight, and that call could be moved outside the
critical section, but we also have this in AbsortFsyncRequests():

> /*
> * We have to PANIC if we fail to absorb all the pending requests (eg,
> * because our hashtable runs out of memory). This is because the system
> * cannot run safely if we are unable to fsync what we have been told to
> * fsync. Fortunately, the hashtable is so small that the problem is
> * quite unlikely to arise in practice.
> */
> START_CRIT_SECTION();

Perhaps we could fix that by just calling fsync() directly if the
allocation fails, like we do if ForwardFsyncRequest() fails.

But if we give the checkpointer process a free pass, running the
regression tests with an assertion in AllocSetAlloc catches five genuine
bugs:

1. _bt_newroot
2. XLogFileInit
3. spgPageIndexMultiDelete
4. PageRepairFragmentation
5. PageIndexMultiDelete

I'll commit the attached patch to fix those shortly.

- Heikki