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

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
nonnull column values,  
     and in old tuple de-toasted columns are not getting freed, if this repeats for more number of tuples there is
chanceof 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
numAttrsis 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
largenumber of columns.           
3. There are visible space savings when number of columns containing NULLS are more than > 60~70% in table have large
numberof columns.  


With Regards,
Amit Kapila.