Re: [POC] A better way to expand hash indexes.

On Thu, Mar 30, 2017 at 2:36 PM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
> Thanks Robert, I have tried to fix the comments given as below.

Thanks.

Since this changes the on-disk format of hash indexes in an
incompatible way, I think it should bump HASH_VERSION. (Hopefully
that doesn't preclude REINDEX?) pg_upgrade should probably also be
patched to issue a warning if upgrading from a version < 10 to a
version >= 10 whenever hash indexes are present; I thought we had
similar cases already, but I don't see them at the moment. Maybe we
can get Bruce or someone to give us some advice on exactly what should
be done here.

In a couple of places, you say that a splitpoint group has 4 slots
rather than 4 phases.

I think that in _hash_get_totalbuckets(), you should use blah &
HASH_SPLITPOINT_PHASE_MASK rather than blah %
HASH_SPLITPOINT_PHASES_PER_GRP for consistency with _hash_spareindex
and, perhaps, speed. Similarly, instead of blah /
HASH_SPLITPOINT_PHASES_PER_GRP, use blah >>
HASH_SPLITPOINT_PHASE_BITS.

buckets_toadd is punctuated oddly. buckets_to_add? Instead of
hand-calculating this, how about calculating it as
_hash_get_totalbuckets(spare_ndx) - _hash_get_totalbuckets(spare_ndx -
1)? That way you reuse the existing logic instead of writing a
slightly different thing in a new place and maybe making a mistake.
If you're going to calculate it, use & and >> rather than % and /, as
above, and drop the parentheses around new_bucket -- this isn't a
macro definition.

+ uint32 splitpoint_group = 0;

Don't need the = 0 here; the next reference to this variable is an
unconditional initialization.

+ */
+
+ splitpoint_group = HASH_SPLITPOINT_PHASE_TO_SPLITPOINT_GRP(spare_ndx);

I would delete the blank line.

- * should start from ((2 ^ i) + metap->hashm_spares[i - 1] + 1).
+ * should start from
+ * (_hash_get_totalbuckets(i) + metap->hashm_spares[i - 1] + 1).

Won't survive pgindent.

- * The number of buckets in the new splitpoint is equal to the total
- * number already in existence, i.e. new_bucket. Currently this maps
- * one-to-one to blocks required, but someday we may need a more
- * complicated calculation here. We treat allocation of buckets as a
- * separate WAL-logged action. Even if we fail after this operation,
- * won't leak bucket pages; rather, the next split will consume this
- * space. In any case, even without failure we don't use all the space
- * in one split operation.
+ * The number of buckets in the new splitpoint group is equal to the
+ * total number already in existence. But we do not allocate them at
+ * once. Each splitpoint group will have 4 slots, we distribute the
+ * buckets equally among them. So we allocate only one fourth of total
+ * buckets in new splitpoint group at a time to consume one phase after
+ * another. We treat allocation of buckets as a separate WAL-logged
+ * action. Even if we fail after this operation, won't leak bucket
+ * pages; rather, the next split will consume this space. In any case,
+ * even without failure we don't use all the space in one split
+ * operation.

I think here you should break this into two paragraphs -- start a new
paragraph with the sentence that begins "We treat..."

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