Hello, I looked into the patch and have some comments.
>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.
==== 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.
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.
- 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.
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.
==== 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.
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.
...
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
