|From:||Amit kapila <amit(dot)kapila(at)huawei(dot)com>|
|To:||"robertmhaas(at)gmail(dot)com" <robertmhaas(at)gmail(dot)com>, "josh(at)agliodbs(dot)com" <josh(at)agliodbs(dot)com>|
|Subject:||Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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)
> 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.
- 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;
for (i = 0; i < numAttrs; i++) <== Tailing toasted value wouldn't be freed as updated to NULL and numAttrs is modified to smaller value.
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.
|Next Message||Robert Haas||2012-10-15 14:10:29||Re: Deparsing DDL command strings|
|Previous Message||Andrew Dunstan||2012-10-15 13:43:51||Re: Deprecating RULES|