Hitoshi Harada <umi.tan...@gmail.com> writes:
> On Thu, Oct 18, 2012 at 8:35 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> I'm not terribly comfortable with trying to use a PG_TRY block to catch
>> an OOM error - there are too many ways that could break, and this code
>> path is by definition not very testable. I think moving up the
>> expand_table action is probably the best bet. Will you submit a patch?
> Here it is. I factored out the bucket finding code to re-calculate it
> after expansion.
I didn't like that too much. I think a better solution is just to do
the table expansion at the very start of the function, along the lines
of the attached patch. This requires an extra test of the "action"
parameter, but I think that's probably cheaper than an extra function
call. It's definitely cheaper than recalculating the hash etc when
a split does occur.
regards, tom lane
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 6d0e0f5366b5a733af9bc712d8ccdb9850721f98..31ac2b25e8ffa2f50dfa4c9bde0d248baf2c0979 100644
*** a/src/backend/utils/hash/dynahash.c
--- b/src/backend/utils/hash/dynahash.c
***************
*** 21,26 ****
--- 21,31 ----
* lookup key's hash value as a partition number --- this will work because
* of the way calc_bucket() maps hash values to bucket numbers.
*
+ * For hash tables in shared memory, the memory allocator function should
+ * match malloc's semantics of returning NULL on failure. For hash tables
+ * in local memory, we typically use palloc() which will throw error on
+ * failure. The code in this file has to cope with both cases.
+ *
* Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
*************** hash_search_with_hash_value(HTAB *hashp,
*** 821,826 ****
--- 826,852 ----
#endif
/*
+ * If inserting, check if it is time to split a bucket.
+ *
+ * NOTE: failure to expand table is not a fatal error, it just means we
+ * have to run at higher fill factor than we wanted. However, if we're
+ * using the palloc allocator then it will throw error anyway on
+ * out-of-memory, so we must do this before modifying the table.
+ */
+ if (action == HASH_ENTER || action == HASH_ENTER_NULL)
+ {
+ /*
+ * Can't split if running in partitioned mode, nor if frozen, nor if
+ * table is the subject of any active hash_seq_search scans. Strange
+ * order of these tests is to try to check cheaper conditions first.
+ */
+ if (!IS_PARTITIONED(hctl) && !hashp->frozen &&
+ hctl->nentries / (long) (hctl->max_bucket + 1) >= hctl->ffactor &&
+ !has_seq_scans(hashp))
+ (void) expand_table(hashp);
+ }
+
+ /*
* Do the initial lookup
*/
bucket = calc_bucket(hctl, hashvalue);
*************** hash_search_with_hash_value(HTAB *hashp,
*** 940,963 ****
currBucket->hashvalue = hashvalue;
hashp->keycopy(ELEMENTKEY(currBucket), keyPtr, keysize);
- /* caller is expected to fill the data field on return */
-
/*
! * Check if it is time to split a bucket. Can't split if running
! * in partitioned mode, nor if table is the subject of any active
! * hash_seq_search scans. Strange order of these tests is to try
! * to check cheaper conditions first.
*/
- if (!IS_PARTITIONED(hctl) &&
- hctl->nentries / (long) (hctl->max_bucket + 1) >= hctl->ffactor &&
- !has_seq_scans(hashp))
- {
- /*
- * NOTE: failure to expand table is not a fatal error, it just
- * means we have to run at higher fill factor than we wanted.
- */
- expand_table(hashp);
- }
return (void *) ELEMENTKEY(currBucket);
}
--- 966,977 ----
currBucket->hashvalue = hashvalue;
hashp->keycopy(ELEMENTKEY(currBucket), keyPtr, keysize);
/*
! * Caller is expected to fill the data field on return. DO NOT
! * insert any code that could possibly throw error here, as doing
! * so would leave the table entry incomplete and hence corrupt the
! * caller's data structure.
*/
return (void *) ELEMENTKEY(currBucket);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
