Re: pageinspect patch, for showing tuple data
От | Guillaume Lelarge |
---|---|
Тема | Re: pageinspect patch, for showing tuple data |
Дата | |
Msg-id | CAECtzeXqK5Ke+AfOTADMsN2c4DsS0yDCgeBYMy8yrcOKz0+i7g@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: pageinspect patch, for showing tuple data (Michael Paquier <michael.paquier@gmail.com>) |
Список | 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.
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Victor WagnerДата:
Сообщение: Re: Patch (3): Implement failover on libpq connect level.
Следующее
От: Masahiko SawadaДата:
Сообщение: Re: Support for N synchronous standby servers - take 2