Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]

On Saturday, October 13, 2012 1:24 PM Amit kapila wrote:
Tue, 26 Jun 2012 17:04:42 -0400 Robert Haas wrote:

>> I see you posted up a follow-up email asking Tom what he had in mind.
>> Personally, I don't think this needs incredibly complicated testing.
>> I think you should just test a workload involving inserting and/or
>> updating rows with lots of trailing NULL columns, and then another
>> workload with a table of similar width that... doesn't.  If we can't
>> find a regression - or, better, we find a win in one or both cases -
>> then I think we're done here.

>As per the last discussion for this patch, performance data needs to be provided before this patch's Review can proceed >further.
>So as per your suggestion and from the discussions about this patch, I have collected the performance data as below:

>Results are taken with following configuration. 
>1. Schema - UNLOGGED TABLE with 2,000,000 records having all columns are INT type. 
>2. shared_buffers = 10GB 
>3. All the performance result are taken with single connection. 
>4. Performance is collected for INSERT operation (insert into temptable select * from inittable) 

>Platform details: 
>    Operating System: Suse-Linux 10.2 x86_64 
>    Hardware : 4 core (Intel(R) Xeon(R) CPU L5408 @ 2.13GHz) 
>    RAM : 24GB 

Further to Performance data, I have completed the review of the Patch.

Basic stuff: 
------------ 
        - Rebase of Patch is required. 
                As heap_fill_tuple function prototype is moved to different file [htup.h to htup_details.h] 
        - Compiles cleanly without any errors/warnings 
        - Regression tests pass. 


Code Review comments: 
--------------------- 
1. There is possibility of memory growth in case of toast table, if trailing toasted columns are updated to NULLs; 
i.e. In Function toast_insert_or_update, for tuples when 'need_change' variable is true, numAttrs are modified to last non null column values, 
     and in old tuple de-toasted columns are not getting freed, if this repeats for more number of tuples there is chance of out of memory. 

        if (        need_change) 
        { 
                numAttrs = lastNonNullValOffset + 1; 
         .... 
        } 

        if (need_delold) 
                for (i = 0; i < numAttrs; i++) <== Tailing toasted value wouldn't be freed as updated to NULL and numAttrs is modified to smaller value. 
                        if (toast_delold[i]) 
                                toast_delete_datum(rel, toast_oldvalues[i]); 

2. Comments need to updated in following functions; how ending null columns are skipped in header part. 
        heap_fill_tuple - function header 
        heap_form_tuple, heap_form_minimal_tuple, heap_form_minimal_tuple. 

3. Why following change is required in function toast_flatten_tuple_attribute 
        -        numAttrs = tupleDesc->natts; 
      +        numAttrs = HeapTupleHeaderGetNatts(olddata); 


Detailed Performance Report for Insert and Update Operations is attached with this mail.

Observations from Performance Results
------------------------------------------------
1. There is no performance change for cloumns that have all valid values(non- NULLs).
2. There is a visible performance increase when number of columns containing NULLS are more than > 60~70% in table have large number of columns.          
3. There are visible space savings when number of columns containing NULLS are more than > 60~70% in table have large number of columns. 


With Regards,
Amit Kapila.