On Fri, Feb 3, 2017 at 4:36 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Fri, Feb 3, 2017 at 9:46 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: >> Robert Haas <robertmh...@gmail.com> writes: >>> On Thu, Feb 2, 2017 at 11:00 PM, Tom Lane <t...@sss.pgh.pa.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.
Removed_platform_specific_fields_metapagege_info_func.patch
Description: invalid/octet-stream
-- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
