Re: Performance Improvement by reducing WAL for Update Operation

On Friday, December 07, 2012 2:28 PM Kyotaro HORIGUCHI wrote:

> Hello, I looked into the patch and have some comments.

Thank you for reviewing the patch.

> From the restriction of the time for this rather big patch,
> please excuse that these comments are on a part of it. Others
> will follow in few days.

It's perfectly fine.


>==== heaptuple.c 
> 
>noncachegetattr(_with_len): 
> 
>- att_getlength should do strlen as worst case or VARSIZE_ANY 
>  which is heavier than doing one comparizon, so I recommend to 
>  add 'if (len)' as the restriction for doing this, and give NULL 
>  as &len to nocachegetattr_with_len in nocachegetattr. 
Fixed. 

>heap_attr_get_length_and_check_equals: 
> 
>- Size seems to be used conventionary as the type for memory 
>  object length, so it might be better using Size instead of 
>  int32 as the type for *tup[12]_attr_len in parameter. 

Fixed. 

>- This function returns always false for attrnum <= 0 as whole 
>  tuple or some system attrs comparison regardless of the real 
>  result, which is a bit different from the anticipation which 
>  the name gives. If you need to keep this optimization, it 
>  should have the name more specific to the purpose. 

The heap_attr_get_length_and_check_equals function is similar to heap_tuple_attr_equals, 
the attrnum <= 0 check is required for heap_tuple_attr_equals. 

>haap_delta_encode: 
> 
>- Some misleading variable names (like match_not_found), 
>  some reatitions of similiar codelets (att_align_pointer, pglz_out_tag), 
>  misleading slight difference of the meanings of variables of 
>  similar names(old_off and new_off and the similar pairs), 
>  and bit tricky use of pglz_out_add and pglz_out_tag with length = 0. 
> 
>  These are welcome to be modified for better readability. 

The variable names are modified, please check them once. 

The (att_align_pointer, pglz_out_tag) repetition code is added to take care of padding only incase of values are equal. 
Use of pglz_out_add and pglz_out_tag with length = 0 is done because of code readability. 

>==== heapam.c 
> 
>fastgetattr_with_len 
> 
>- Missing left paren in the line 867 ('nocachegetattr_with_len(tup)...') 
> 
>- Missing enclosing paren in heapam.c:879 (len, only on style) 
> 
>- Allowing len = NULL will be good for better performance, like 
>  noncachegetattr. 

Fixed. except len=NULL because fastgetattr is modified as below comment. 

>fastgetattr 
> 
>- I suppose that the coding covension here is that macro and 
>  alternative c-code are expected to be look similar. fastgetattr 
>  looks quite differ to corresponding macro.

Fixed. 


Another change is also done to handle the history size of 2 bytes which is possible with the usage of LZ macro's for delta encoding.


With Regards,
Amit Kapila.