From: | Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com>, Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pageinspect: Hash index support |
Date: | 2017-02-02 11:29:04 |
Message-ID: | CAE9k0Pnkx0mxAejDX6S6EBaNAgv=Fj9DoEx7B0vy=d6kmzcNzQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
>
> + ptr = (char *) itup + IndexInfoFindDataOffset(itup->t_info);
> + Assert(ptr <= uargs->page + BLCKSZ);
>
> I think this should be promoted to an ereport(); these functions can
> accept an arbitrary bytea.
I think we had added 'ptr' variable initially just to dump hash code
in hexadecimal format but now since we have removed that logic from
current code, I think it is no more required and I have therefore
removed it from the current patch. Below is the code that used it
initially. It got changed as per your comment - [1]
>
> + if (opaque->hasho_flag & LH_BUCKET_PAGE)
> + stat->hasho_prevblkno = InvalidBlockNumber;
> + else
> + stat->hasho_prevblkno = opaque->hasho_prevblkno;
>
> I think we should return the raw value here. Mithun's patch to cache
> the metapage hasn't gone in yet, but even if it does, let's assume
> anyone using contrib/pageinspect wants to see the data that's
> physically present, not our gloss on it.
okay, agreed. I have corrected this in the attached v10 patch.
>
> Other than that, I don't think I have any other comments on this. The
> tests that got added look a little verbose to me (do we really need to
> check pages 1-4 separately in each case when they're all hash pages?
> if hash_bitmap_info rejects one, surely it will reject the others) but
> I'm not going to fight too hard if Peter wants it that way.
>
I think it's OK to check hash_bitmap_info() or any other functions
with different page types at least once.
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
0001-Add-support-for-hash-index-in-pageinspect-contrib-mo_v10.patch | invalid/octet-stream | 34.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | amul sul | 2017-02-02 12:09:54 | Constraint exclusion failed to prune partition in case of partition expression involves function call |
Previous Message | Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?= | 2017-02-02 10:44:54 | Re: [PATCH] Add tab completion for DEALLOCATE |