Re: [HACKERS] pageinspect: Hash index support

On Tue, Jan 10, 2017 at 12:19 PM, Jesper Pedersen
<jesper.pedersen@redhat.com> wrote:
> Revised patched attached.

+        itup = (IndexTuple) PageGetItem(uargs->page, id);
+
+        MemSet(nulls, 0, sizeof(nulls));
+
+        j = 0;
+        values[j++] = UInt16GetDatum(uargs->offset);
+        values[j++] = CStringGetTextDatum(psprintf("(%u,%u)",
+
BlockIdGetBlockNumber(&(itup->t_tid.ip_blkid)),
+                                            itup->t_tid.ip_posid));
+
+        ptr = (char *) itup + IndexInfoFindDataOffset(itup->t_info);
+        dlen = IndexTupleSize(itup) - IndexInfoFindDataOffset(itup->t_info);

It seems like this could be used to index off the end of the page, if
you feed it invalid data.

+        dump = palloc0(dlen * 3 + 1);

This is wasteful.  Just use palloc and install a terminating NUL byte instead.

+            sprintf(dump, "%02x", *(ptr + off) & 0xff);

*(ptr + off) is normally written ptr[off].

+    if (pageopaque->hasho_flag != LH_OVERFLOW_PAGE)
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("page is not an overflow page"),
+                 errdetail("Expected %08x, got %08x.",
+                            LH_OVERFLOW_PAGE, pageopaque->hasho_flag)));

I think this is an unnecessary test given that you've already called
verify_hash_page().

+    if (bitmappage >= metap->hashm_nmaps)
+        elog(ERROR, "invalid overflow bit number %u", ovflbitno);

I think this should be an ereport(), because it's reachable given a
bogus page which a user might construct (or a corrupted page).

+test=# SELECT * FROM hash_page_items(get_raw_page('con_hash_index', 1));
+ itemoffset |      ctid       |          data
+------------+-----------------+-------------------------
+          1 | (3145728,14376) | 00 c0 ca 3e 00 00 00 00
+          2 | (3145728,14376) | 00 c0 ca 3e 00 00 00 00
+          3 | (3407872,14376) | 00 c0 ca 3e 00 00 00 00

Won't the first 4 bytes always be a hash code and the second 4 bytes
always 0?  Should we just return the hash code as an int4 or int8
instead of pretending it's a bunch of uninterpretable binary data?

+      <function>hash_bitmap_info</function> returns information about
+      the status of a bit for an overflow page in bitmap page of a
<acronym>HASH</acronym>
+      index. For example:
+<screen>
+test=# SELECT * FROM hash_bitmap_info('con_hash_index', 2050);
+ bitmapblkno | bitmapbit
+-------------+-----------
+          65 |         1
+</screen>

I find this hard to understand.  This says that it returns
information, but the nature of the returned information is unspecified
and in my opinion unclear.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company