From: | Amit kapila <amit(dot)kapila(at)huawei(dot)com> |
---|---|
To: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | "hlinnakangas(at)vmware(dot)com" <hlinnakangas(at)vmware(dot)com>, "noah(at)leadboat(dot)com" <noah(at)leadboat(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Performance Improvement by reducing WAL for Update Operation |
Date: | 2012-12-07 13:06:23 |
Message-ID: | 6C0B27F7206C9E4CA54AE035729E9C383BEA1C90@szxeml509-mbx |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
Attachment | Content-Type | Size |
---|---|---|
wal_update_changes_lz_v5.patch | application/octet-stream | 47.3 KB |
wal_update_changes_mod_lz_v6.patch | application/octet-stream | 51.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2012-12-07 13:33:21 | Re: Support for REINDEX CONCURRENTLY |
Previous Message | Pavan Deolasee | 2012-12-07 12:47:47 | Re: Setting visibility map in VACUUM's second phase |