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

В письме от 26 сентября 2015 20:57:25 Вы написали:

> >> 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 one with the most arguments. Now perhaps we could have as well two
> of them: one which has the minimum number of arguments with default
> values, and the second one with the maximum number of arguments.

I've been thinking about this warning mode for a while...

I've added this argument to the heap_page_item_atts, because if somebody would
like to force it to parse fake data, it would be really difficult to provide
proper fake data if one heap to perfectly fit tuple descriptor of another heap.
So to do this you have to lover error level.

But since now we actually parse data with tuple_data_split, we can provide a
precisely formed  fake information, so you are not limited with how it is
actually stored in page. You just pass any arguments you want. So you does not
need warning mode anymore.

So may be I should get rid of "warning mode" now.

Concerning do_detoast argument. I do not have any clear opinion. I thing that
tuple_data_split is a kind of internal function. So it is ok if it is not so
convenient to use.

But in  heap_page_item_attrs should be optional. Because it is used from plsql
console, and in most cases user does not want deTOASing.

So if in one function do_detoast is optional, may be it would be good to have
it optional in other functions to keep similarity...

So summorising: if you want make things simpler, I would first get totally rid
of warning_mode first of all, then if you would insist, make do_detoast non-
optional.

>
> >> +        <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.
>
> pageinspect is already something that works at low-level. My guess is
> that users of this module are going to have a look at the code either
> way to understand how tuple data is manipulated within a page.


> > So I would consider two options: Either move t_infomask/t_infomask2
> > details
> > into storage.sgml or leave as it is.
>
> The documentation redirects the reader to look at htup_details.h (the
> documentation is actually incorrect, I sent a separate patch)
I do not see any redirect at
http://www.postgresql.org/docs/9.4/static/pageinspect.html

> , and I
> see no point in duplicating such low-level descriptions, that would be
> double maintenance for the same result.
>
> > I am lazy, and does not feel confidence about touching main documentation,
> > so I would prefer to leave as it is.
>
> Your patch is proving the contrary :) Honestly I would just rip out
> the part you have added to describe all the fields related to
> HeapTupleHeaderData, and have only a simple self-contained example to
> show how to use the new function tuple_data_split.
May be. I would think about it a little bit more, and discuss it with my
friends

> >> 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
>
> I would as well just remove heap_page_item_attrs, an example in the
> docs would be just enough IMO (note that I never mind being outvoted).

This is function that pageinspect user is definitely will use this function.
And as a user I would like to see function output in documentation the way I
would see it, in order to be better prepared to see it in real life. There are
limits however, get_raw_page output should not be shown in documentation ;-)
heap_page_item_attrs is close to these limits, but did not reach it, in my
opinion... So I'd prefer to keep it there.

> Btw, shouldn't the ereport messages in tuple_data_split use
> error_level instead of ERROR?
These errors are in the part where I parse t_bits from text back to bit
representation.  Here there is not chance to get non-strict behavior, since
there is no way to guess what should we do if we met characters then '0' or
'1'. Or what to do if there is only 7 characters but we should write whole
byte.etc...
Moreover as I wrote above bay be we just do not need warning_mode at all for
current situation.

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