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

>But though the code looks better locally than before, the larger problem
>is that this is still unsafe. As Pavan pointed out, XLogInsert is called
>from inside critical sections, so we can't allocate memory here.
>Could you look into his suggestions of other places to do the
>allocation, please?

If I understand correctly , the reason memory allocation is not allowed in
critical section is because OOM error in critical section can lead to PANIC.
This patch does not report an OOM error on memory allocation failure
instead it proceeds without compression of FPW if sufficient memory is not
available for compression. Also, the memory is allocated just once very
early in the session. So , the probability of OOM seems to be low and even
if it occurs it is handled as mentioned above.
Though Andres said we cannot use malloc in critical section, the memory
allocation done in the patch does not involve reporting OOM error in case
of failure. IIUC, this eliminates the probability of PANIC in critical
section. So, I think keeping this allocation in critical section should be
fine. Am I missing something?

Thank you,
Rahila Syed

On Mon, Jul 7, 2014 at 4:43 PM, Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:

>
> Thank you for review comments.
>
> >There are still numerous formatting changes required, e.g. spaces around
> >"=" and correct formatting of comments. And "git diff --check" still has
> >a few whitespace problems. I won't point these out one by one, but maybe
> >you should run pgindent
>
> I will do this.
>
> >Could you look into his suggestions of other places to do the
> >allocation, please?
>
> I will get back to you on this
>
>
> >Wouldn't it be better to set
>
> > bkpb->block_compression = compress_backup_block;
>
> >once earlier instead of setting it that way once and setting it to
> >BACKUP_BLOCK_COMPRESSION_OFF in two other places
> Yes.
>
> If you're using VARATT_IS_COMPRESSED() to detect compression, don't you
> need SET_VARSIZE_COMPRESSED() in CompressBackupBlock? pglz_compress()
> does it for you, but the other two algorithms don't.
> Yes we need SET_VARSIZE_COMPRESSED. It is present in wrappers around
> snappy and LZ4 namely pg_snappy_compress and pg_LZ4_compress.
>
> >But now that you've added bkpb.block_compression, you should be able to
> >avoid VARATT_IS_COMPRESSED() altogether, unless I'm missing something.
> >What do you think?
> You are right. It can be removed.
>
>
> Thank you,
>
>
>
> On Fri, Jul 4, 2014 at 9:35 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
> wrote:
>
>> At 2014-07-04 21:02:33 +0530, ams(at)2ndQuadrant(dot)com wrote:
>> >
>> > > +/*
>> > > + */
>> > > +static const struct config_enum_entry
>> backup_block_compression_options[] = {
>>
>> Oh, I forgot to mention that the configuration setting changes are also
>> pending. I think we had a working consensus to use full_page_compression
>> as the name of the GUC. As I understand it, that'll accept an algorithm
>> name as an argument while we're still experimenting, but eventually once
>> we select an algorithm, it'll become just a boolean (and then we don't
>> need to put algorithm information into BkpBlock any more either).
>>
>> -- Abhijit
>>
>
>