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

So what's next?
We need something else to discuss?
We need somebody else's opinion to rule this out?
Or it's ready to commit, and just not marked this way?

I am going to make report based on this patch in Vienna. It would be
nice to have it committed until then :)

On 02.10.2015 07:10, Michael Paquier wrote:
> On Thu, Oct 1, 2015 at 8:13 PM, Nikolay Shaplov wrote:
>> В письме от 30 сентября 2015 13:49:00 пользователь Michael Paquier
> написал:
>>> -                                 errmsg("input page too small (%d
> bytes)",
>>> raw_page_size)));
>>> +                                        errmsg("input page too small (%d
>>> bytes)", raw_page_size)));
>>> Please be careful of spurious diffs. Those just make the life of
> committers
>>> more difficult than it already is.
>> Oh, that's not diff. That's I've fixed indent on the code I did not write
> :-)
>
> pgindent would have taken care of that if needed. There is no need to add
> noise in the code for this patch.
>
>>> +     <para>
>>> +     General idea about output columns: <function>lp_*</function>
>>> attributes
>>> +     are about where tuple is located inside the page;
>>> +     <function>t_xmin</function>, <function>t_xmax</function>,
>>> +     <function>t_field3</function>, <function>t_ctid</function> are
> about
>>> +     visibility of this tuplue inside or outside of the transaction;
>>> +     <function>t_infomask2</function>, <function>t_infomask</function>
> has
>>> some
>>> +     information about properties of attributes stored in tuple data.
>>> +     <function>t_hoff</function> sais where tuple data begins and
>>> +     <function>t_bits</function> sais which attributes are NULL and
> which
>>> are
>>> +     not. Please notice that t_bits here is not an actual value that is
>>> stored
>>> +     in tuple data, but it's text representation  with '0' and '1'
>>> charactrs.
>>> +     </para>
>>> I would remove that as well. htup_details.h contains all this
> information.
>> Made it even more shorter. Just links and comments about representation of
>> t_bits.
> I would completely remove this part.
>
>> There also was a bug in original pageinspect, that did not get t_bits
> right
>> when there was OID in the tuple.  I've fixed it too.
> Aha. Good catch! By looking at HeapTupleHeaderGetOid if the tuple has an
> OID it will be stored at the end of HeapTupleHeader and t_hoff includes it,
> so bits_len is definitely larger of 4 bytes should an OID be present.
>                if (tuphdr->t_infomask & HEAP_HASNULL)
>                {
> -                       bits_len = tuphdr->t_hoff -
> -                           offsetof(HeapTupleHeaderData, t_bits);
> +                      int     bits_len =
> +                          ((tuphdr->t_infomask2 & HEAP_NATTS_MASK)/8 +
> 1)*8;
>
>                         values[11] = CStringGetTextDatum(
> -                                           bits_to_text(tuphdr->t_bits,
> bits_len * 8));
> +                                          bits_to_text(tuphdr->t_bits,
> bits_len));
>                                 }
> And here is what you are referring to in your patch. I think that we should
> instead check for HEAP_HASOID and reduce bits_len by the size of Oid should
> one be present. As this is a bug that applies to all the existing versions
> of Postgres it would be good to extract it as a separate patch and then
> apply your own patch on top of it instead of including in your feature.
> Attached is a patch, this should be applied down to 9.0 I guess. Could you
> rebase your patch on top of it?
>
>> Here is next patch in attachment.
> Cool, thanks.
>
> -test=# SELECT * FROM heap_page_items(get_raw_page('pg_class', 0));
> +
> +test=# select * from heap_page_items(get_raw_page('pg_range', 0));
> This example in the docs is far too long in character length... We should
> get that reduced.
>
> +      Please notice that <function>t_bits</function> in tuple header
> structure
> +      is a binary bitmap, but <function>heap_page_items</function> returns
> +      <function>t_bits</function> as human readable text representation of
> +      original <function>t_bits</function> bitmap.
> This had better remove the mention to "please notice". Still as this is
> already described in htup_details.h there is no real point to describe it
> again here: that's a bitmap of NULLs.
>
>