Re: pageinspect patch, for showing tuple data - Mailing list pgsql-hackers

<p dir="ltr">Hi,<p dir="ltr">Le 12 nov. 2015 1:05 AM, "Michael Paquier" <<a
href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>a écrit :<br /> ><br /> > On Thu, Nov
12,2015 at 12:41 AM, Nikolay Shaplov<br /> > <<a
href="mailto:n.shaplov@postgrespro.ru">n.shaplov@postgrespro.ru</a>>wrote:<br /> > > В письме от 28 октября
201516:57:36 пользователь Michael Paquier написал:<br /> > >> On Sat, Oct 17, 2015 at 1:48 AM, Michael Paquier
wrote:<br/> > >> > On Sat, Oct 17, 2015 at 5:15 AM, Nikolay Shaplov wrote:<br /> > >> >> Or
it'sready to commit, and just not marked this way?<br /> > >> ><br /> > >> > No, I don't think
wehave reached this state yet.<br /> > >> ><br /> > >> >> I am going to make report based on
thispatch in Vienna. It would be<br /> > >> >> nice to have it committed until then :)<br /> >
>>><br /> > >> > This is registered in this November's CF and the current one is not<br /> >
>>> completely wrapped up, so I haven't rushed into looking at it.<br /> > >><br /> > >> So,
Ihave finally been able to look at this patch in more details,<br /> > >> resulting in the attached. I noticed
acouple of things that should be<br /> > >> addressed, mainly:<br /> > >> - addition of a new routine
text_to_bitsto perform the reverse<br /> > >> operation of bits_to_text. This was previously part of<br />
>>> tuple_data_split, I think that it deserves its own function.<br /> > >> - split_tuple_data should
bestatic<br /> > >> - t_bits_str should not be a text, rather a char* fetched using<br /> > >>
text_to_cstring(PG_GETARG_TEXT_PP(4)).This way there is no need to<br /> > >> perform extra calculations with
VARSIZEand VARHDRSZ<br /> > >> - split_tuple_data can directly use the relation OID instead of the<br /> >
>>tuple descriptor of the relation<br /> > >> - t_bits was leaking memory. For correctness I think that
itis better<br /> > >> to free it after calling split_tuple_data.<br /> > >> - PG_DETOAST_DATUM_COPY
allocatessome memory, this was leaking as<br /> > >> well in raw_attr actually. I refactored the code such as
abytea* is<br /> > >> used and always freed when allocated to avoid leaks. Removing raw_attr<br /> >
>>actually simplified the code a bit.<br /> > >> - I simplified the docs, that was largely too verbose
inmy opinion.<br /> > >> - Instead of using VARATT_IS_1B_E and VARTAG_EXTERNAL, using<br /> > >>
VARATT_IS_EXTERNALand VARATT_IS_EXTERNAL_ONDISK seems more adapted to<br /> > >> me, those other ones are much
morelow-level and not really spread in<br /> > >> the backend code.<br /> > >> - Found some typos in
thecode, the docs and some comments. I reworked<br /> > >> the error messages as well.<br /> > >> -
Thecode format was not really in line with the project guidelines.<br /> > >> I fixed that as well.<br /> >
>>- I removed heap_page_item_attrs for now to get this patch in a<br /> > >> bare-bone state. Though I
wouldnot mind if this is re-added (I<br /> > >> personally don't think that's much necessary in the module<br
/>> >> actually...).<br /> > >> - The calculation of the length of t_bits using HEAP_NATTS_MASK is<br
/>> >> more correct as you mentioned earlier, so I let it in its state.<br /> > >> That's actually
moreaccurate for error handling as well.<br /> > >> That's everything I recall I have. How does this look?<br
/>> > You've completely rewrite everything ;-)<br /> > ><br /> > > Let everything be the way you
wrote.This code is better than mine.<br /> > ><br /> > > With one exception. I really  need
heap_page_item_attrsfunction. I used it in<br /> > > my Tuples Internals presentation<br /> > > <a
href="https://github.com/dhyannataraj/tuple-internals-presentation">https://github.com/dhyannataraj/tuple-internals-presentation</a><br
/>> > and I am 100% sure that this function is needed for educational purposes, and<br /> > > this function
shouldbe as simple as possible, so students can use it without<br /> > > extra efforts.<br /> ><br /> >
Fine.That's your patch after all.<br /> ><br /> > > I still have an opinion that documentation should be more
verbose,than your<br /> > > version, but I can accept your version.<br /> ><br /> > I am not sure that's
necessary,pageinspect is for developers.<br /> ><p dir="ltr">FWIW, I agree that pageinspect is mostly for devs.
Still,as i said to Nikolay after his talk at <a href="http://pgconf.eu">pgconf.eu</a>, it's a nice tool for people who
liketo know what's going on deep inside PostgreSQL.<p dir="ltr">So +1 for that nice feature.<p dir="ltr">> > Who
isgoing to add heap_page_item_attrs to your patch? me or you?<br /> ><br /> > I recall some parts of the code I
stilldid not like much :) I'll grab<br /> > some room to have an extra look at it.