Hi, > 1. > +static Page > +verify_hash_page(bytea *raw_page, int flags) > > Few checks for meta page are missing, refer _hash_checkpage.
okay, I have added the checks for meta page as well. Please refer to attached patch. > > 2. > + if (!superuser()) > + ereport(ERROR, > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > + (errmsg("must be superuser to use pageinspect functions")))); > > Isn't it better to use "raw page" instead of "pageinspect" in the > above error message? If you agree, then fix other similar occurrences > in the patch. Yes, knowing that we deal with raw pages as in brin index it would be good to use raw page instead of pageinspect to maintain the consistency in the error message. > > 3. > + values[j++] = CStringGetTextDatum(psprintf("(%u,%u)", > + BlockIdGetBlockNumber(&(itup->t_tid.ip_blkid)), > + itup->t_tid.ip_posid)); > > Fix indentation in the third line. > Corrected. Please check the attached patch. > 4. > +Datum > +hash_page_items(PG_FUNCTION_ARGS) > +{ > + bytea *raw_page = PG_GETARG_BYTEA_P(0); > > > +Datum > +hash_bitmap_info(PG_FUNCTION_ARGS) > +{ > + Oid indexRelid = PG_GETARG_OID(0); > + uint32 ovflblkno = PG_GETARG_UINT32(1); > > Is there a reason for keeping the input arguments for > hash_bitmap_info() different from hash_page_items()? > Yes, there are two reasons behind it. Firstly, we need metapage to identify the bitmap page that holds the information about the overflow page passed as an input to this function. Secondly, we will have to input overflow block number as an input to this function so as to determine the overflow bit number which can be used further to identify the bitmap page. + /* Read the metapage so we can determine which bitmap page to use */ + metabuf = _hash_getbuf(indexRel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); + metap = HashPageGetMeta(BufferGetPage(metabuf)); + + /* Identify overflow bit number */ + ovflbitno = _hash_ovflblkno_to_bitno(metap, ovflblkno); + + bitmappage = ovflbitno >> BMPG_SHIFT(metap); + bitmapbit = ovflbitno & BMPG_MASK(metap); + + if (bitmappage >= metap->hashm_nmaps) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid overflow bit number %u", ovflbitno))); + + bitmapblkno = metap->hashm_mapp[bitmappage]; > 5. > +hash_metapage_info(PG_FUNCTION_ARGS) > { > .. > + spares = palloc0(HASH_MAX_SPLITPOINTS * 5 + 1); > .. > + mapp = palloc0(HASH_MAX_BITMAPS * 5 + 1); > .. > } > > Don't you think we should free the allocated memory in this function? > Also, why are you 5 as a multiplier in both the above pallocs, > shouldn't it be 4? Yes, we should free it. We have used 5 as a multiplier instead of 4 because of ' ' character. Apart from above comments, the attached patch also handles all the comments from Mithun. With Regards, Ashutosh Sharma. EnterpriseDB: http://www.enterprisedb.com
0001-Add-support-for-hash-index-in-pageinspect-contrib-mo.patch
Description: invalid/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers