>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?
>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
>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@2ndquadrant.com> wrote:
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
