Hi, Le 12 nov. 2015 1:05 AM, "Michael Paquier" <michael.paqu...@gmail.com> a écrit : > > On Thu, Nov 12, 2015 at 12:41 AM, Nikolay Shaplov > <n.shap...@postgrespro.ru> wrote: > > В письме от 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 can use it without > > extra efforts. > > Fine. That's your patch after all. > > > I still have an opinion that documentation should be more verbose, than your > > version, but I can accept your version. > > I am not sure that's necessary, pageinspect is for developers. >
FWIW, I agree that pageinspect is mostly for devs. Still, as i said to Nikolay after his talk at pgconf.eu, it's a nice tool for people who like to know what's going on deep inside PostgreSQL. So +1 for that nice feature. > > Who is going to add heap_page_item_attrs to your patch? me or you? > > I recall some parts of the code I still did not like much :) I'll grab > some room to have an extra look at it.