Re: pageinspect patch, for showing tuple data - Mailing list pgsql-hackers
From | Nikolay Shaplov |
---|---|
Subject | Re: pageinspect patch, for showing tuple data |
Date | |
Msg-id | 1519297.8h6Ui5KmvK@nataraj-amd64 Whole thread Raw |
In response to | Re: pageinspect patch, for showing tuple data (Michael Paquier <michael.paquier@gmail.com>) |
Responses |
Re: pageinspect patch, for showing tuple data
|
List | pgsql-hackers |
В письме от 28 октября 2015 16:57:36 пользователь Michael Paquier написал: > On Sat, Oct 17, 2015 at 1:48 AM, Michael Paquier wrote: > > On Sat, Oct 17, 2015 at 5:15 AM, Nikolay Shaplov wrote: > >> Or it's ready to commit, and just not marked this way? > > > > No, I don't think we have reached this state yet. > > > >> I am going to make report based on this patch in Vienna. It would be > >> nice to have it committed until then :) > > > > This is registered in this November's CF and the current one is not > > completely wrapped up, so I haven't rushed into looking at it. > > So, I have finally been able to look at this patch in more details, > resulting in the attached. I noticed a couple of things that should be > addressed, mainly: > - addition of a new routine text_to_bits to perform the reverse > operation of bits_to_text. This was previously part of > tuple_data_split, I think that it deserves its own function. > - split_tuple_data should be static > - t_bits_str should not be a text, rather a char* fetched using > text_to_cstring(PG_GETARG_TEXT_PP(4)). This way there is no need to > perform extra calculations with VARSIZE and VARHDRSZ > - split_tuple_data can directly use the relation OID instead of the > tuple descriptor of the relation > - t_bits was leaking memory. For correctness I think that it is better > to free it after calling split_tuple_data. > - PG_DETOAST_DATUM_COPY allocates some memory, this was leaking as > well in raw_attr actually. I refactored the code such as a bytea* is > used and always freed when allocated to avoid leaks. Removing raw_attr > actually simplified the code a bit. > - I simplified the docs, that was largely too verbose in my opinion. > - Instead of using VARATT_IS_1B_E and VARTAG_EXTERNAL, using > VARATT_IS_EXTERNAL and VARATT_IS_EXTERNAL_ONDISK seems more adapted to > me, those other ones are much more low-level and not really spread in > the backend code. > - Found some typos in the code, the docs and some comments. I reworked > the error messages as well. > - The code format was not really in line with the project guidelines. > I fixed that as well. > - I removed heap_page_item_attrs for now to get this patch in a > bare-bone state. Though I would not mind if this is re-added (I > personally don't think that's much necessary in the module > actually...). > - The calculation of the length of t_bits using HEAP_NATTS_MASK is > more correct as you mentioned earlier, so I let it in its state. > That's actually more accurate for error handling as well. > That's everything I recall I have. How does this look? You've completely rewrite everything ;-) Let everything be the way you wrote. This code is better than mine. With one exception. I really need heap_page_item_attrs function. I used it in my Tuples Internals presentation https://github.com/dhyannataraj/tuple-internals-presentation and I am 100% sure that this function is needed for educational purposes, and this function should be as simple as possible, so students cat use it without extra efforts. I still have an opinion that documentation should be more verbose, than your version, but I can accept your version. Who is going to add heap_page_item_attrs to your patch? me or you? -- Nikolay Shaplov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
pgsql-hackers by date: