On Sat, Sep 5, 2015 at 1:05 AM, Nikolay Shaplov wrote:
В письме от 4 сентября 2015 14:58:29 пользователь Michael Paquier написал: > Documentation is missing, that would be good to have to understand what > each function is intended to do.
I were going to add documentation when this patch is commited, or at least approved for commit. So I would know for sure that function definition would not change, so had not to rewrite it again and again.
I doubt that this patch would be committed without documentation, this is a requirement for any patch.
So if it is possible I would write documentation and test when this patch is already approved.
I'm fine with that. Let's discuss its shape and come up with an agreement. It would have been good to post test cases of what this stuff actually does though, people reviewing this thread are limited to guess on what is tried to be achieved just by reading the code. That's actually where documentation, even a draft of documentation helps a lot in the review to see if what is expected by the developer matches what the code actually does.
> Code has some whitespaces. I've found and removed some. Hope that was all of them...
Yeah, it looks that you took completely rid of them.
In details, this patch introduces two independent concepts:
- add tuple data as a new bytea column of heap_page_items. This is indeed where it should be, and note that this is where the several corruption checks are done by checking the state of the tuple data.
- add heap_page_item_attrs and heap_page_item_detoast_attrs, which is very similar to heap_page_items, at the difference that it can take an OID to be able to use a TupleDesc and return a bytea array with the data of each attribute.
Honestly, heap_page_item_attrs and heap_page_item_detoast_attrs are way too similar to what heap_page_items does, leading to a code maze that is going to make future extensions more difficult, which is what lead to the refactoring your patch does.
Hence, instead of all those complications, why not simply introducing two functions that take as input the tuple data and the OID of the relation, returning those bytea arrays? It seems to be that this would be a more handy interface, and as this is for educational purposes I guess that the addition of the overhead induced by the extra function call would be acceptable.
