Re: [REVIEW] Re: Compression of full-page-writes

On Wed, Jun 18, 2014 at 6:25 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:

> At 2014-06-18 18:10:34 +0530, rahilasyed90(at)gmail(dot)com wrote:
> >
> > palloc() is disallowed in critical sections and we are already in CS
> > while executing this code. So we use malloc().
>
> Are these allocations actually inside a critical section? It seems to me
> that the critical section starts further down, but perhaps I am missing
> something.
>
>
ISTM XLogInsert() itself is called from other critical sections. See
heapam.c for example.

> Second, as Andres says, you shouldn't malloc() inside a critical section
> either; and anyway, certainly not without checking the return value.
>
>
I was actually surprised to see Andreas comment. But he is right. OOM
inside CS will result in a PANIC. I wonder if we can or if we really do
enforce that though. The code within #ifdef WAL_DEBUG in the same function
is surely doing a palloc(). That will be caught since there is an assert
inside palloc(). May be nobody tried building with WAL_DEBUG since that
assert was added.

May be Rahila can move that code to InitXLogAccess or even better check for
malloc() return value and proceed without compression. There is code in
snappy.c which will need similar handling, if we decide to finally add that
to core.

> I am not sure if the change will be a significant improvement from
> > performance point of view except it will save few condition checks.
>
> Moving that allocation out of the outer for loop it's currently in is
> *nothing* to do with performance, but about making the code easier to
> read.
>
>
+1.

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee