Re: pgsql: pageinspect: Try to fix some bugs in previous commit.

On Fri, Feb 3, 2017 at 4:36 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Feb 3, 2017 at 9:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Thu, Feb 2, 2017 at 11:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> I'm about to push a fix that removes the crashes (or at least the ones
>>>> I see on dromedary),
>>
>>> For comparison, a patch I wrote by inspection is attached.
>>
>> Hm, some of what you have here matches what I just pushed, but not all.
>>
>> I just made the C code agree with what the SQL declarations for the
>> functions say. I'm pretty dubious that the SQL declarations are entirely
>> sensible as to which values need to be of what width, but I'll leave that
>> decision for somebody who's been paying closer attention to the hash code.
>>

I think UInt32GetDatum(metad->hashm_procid) looks fine, the reason
being 'hashm_procid' is defined as 32-bit unsigned integer but then we
may have to define procid as int8 in SQL function.

>
> I have gone through all the of the SQL declarations and it seems
> hash_metapage_info(...,OUT procid int4,..) is not consistent. procid
> is unsigned int, so isn't it better to use the corresponding datatype
> as int8 in SQL function hash_metapage_info then use Int64GetDatum?
>
> The other inconsistency in the code appears to be in the usage of
> UInt64GetDatum and Int64GetDatum for same SQL datatype. For ex. SQL
> declaration of hasho_prevblkno is int8 (hash_page_stats(.. , OUT
> hasho_prevblkno int8,..)) and we use Int64GetDatum to fill the
> corresponding C value. However for SQL declaration of maxbucket is
> int8 (hash_metapage_info(..,OUT maxbucket int8,)) and we use
> UInt64GetDatum() to fetch the value. I think it is better to be
> consistent here.

I am sorry but I am not quite able to understand the purpose of
typecasting unsigned integer to signed type at few places and then
using corresponding macros to represent it as Datum. I mean at few
places we have done typecasting of unsigned inetgers to signed type
whereas at some places we have kept it as it is.

>
>>>> I think probably both of those are unavoidable 32-bit v 64-bit
>>>> differences due to available space on a page changing with MAXALIGN.
>>>> What do you want to do about those?
>>
>>> How about we have the test just select a named list of fields instead
>>> of selecting *?
>>
>> Yeah, that's one possible answer. We could also maintain two
>> expected-files for 32 bit v 64 bit, but I'm not sure it's worth
>> the trouble.
>>
>
> I think for now selecting named fields is sufficient.

+1. Attached is the patch that has this changes.

Note: I am extremely sorry for wrongly choosing some of the SQL types
in the patch for pageinspect. I think there were few platform specific
things that too should have been addressed by me. Moreover, I feel
being the owner of this project I should have participated in this
discussion a bit earlier but as I was not subscribed to
pgsql-committers list I could not be on time.