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.