Re: pageinspect patch, for showing tuple data

В письме от 25 сентября 2015 20:59:29 пользователь Michael Paquier написал:

> > Here is final version with documentation.
>
> Thanks! I just had a short look at it:
> - I am not convinced that it is worth declaring 3 versions of
> tuple_data_split.
How which of them should we leave?

> - The patch does not respect the project code style,
> particularly one-line "if condition {foo;}" are not adapted, code line is
limited
> to 80 characters, etc. The list is basically here:
> http://www.postgresql.org/docs/current/static/source.html
I did my best. Results are attached.

> - Be aware of typos: s/whitch/which is one.
I've run spellchecker on all comments. Hope that I removed most of the
mistakes. But as I am not native speaker, I will not be able to eliminate them
all. I will need help here.

> +        <entry><structfield>t_infomask2</structfield></entry>
> +        <entry><type>integer</type></entry>
> +        <entry>stores number of attributes (11 bits of the word). The
> rest are used for flag bits:
> +<screen>
> +0x2000 - tuple was updated and key cols modified, or tuple deleted
> +0x4000 - tuple was HOT-updated
> +0x8000 - this is heap-only tuple
> +0xE000 - visibility-related bits (so called "hint bits")
> +</screen>
> This large chunk of documentation is a duplicate of storage.sgml. If
> that's really necessary, it looks adapted to me to have more detailed
> comments at code level directly in heapfuncs.c.
Hm... There is no explanation of t_infomask/t_infomask2 bits in storage.sgml.

If there is no source of information other then source code, then the
documentation is not good.

If there were information about t_infomask/t_infomask2 in storage.sgml, then I
would add "See storage.sgml for more info" into pageinspect doc, and thats
all. But since there is no such information there, I  think that the best
thing is to quote comments from source code there, so you can get all
information from documentation, not looking for it in the code.

So I would consider two options: Either move t_infomask/t_infomask2 details
into storage.sgml or leave as it is.

I am lazy, and does not feel confidence about touching main documentation, so I
would prefer to leave as it is.


> The example of tuple_data_split in the docs would be more interesting
> if embedded with a call to heap_page_items.

This example would be almost not readable. May be I should add one more praise
explaining where did we take arguments for tuple_data_split


--
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company