Re: pageinspect patch, for showing tuple data

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.

Regards,
--
Michael