Re: [HACKERS] pageinspect: Hash index support

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers