Re: [HACKERS] Small patch: Change calling convention for ShmemInitHash (and fix possible bug)
> In short: the error in Aleksander's argument is the assumption that > shared hashtables have fixed size. That's simply false. Well this is a bit embarrassing but I have to admit that you are right. Dynahash code is a bit non-trivial to say the least (let me guess - there is no point of suggesting a patch that splits it into two or three separate implementations for each use case, right? :) and I misunderstood how it actually works. My apologies. -- Best regards, Aleksander Alekseev http://eax.me/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small patch: Change calling convention for ShmemInitHash (and fix possible bug)
Michael Paquier writes: > On Fri, Mar 25, 2016 at 9:17 PM, Robert Haas wrote: >> On Thu, Mar 24, 2016 at 9:50 AM, Aleksander Alekseev >> wrote: >>> Currently this procedure has two arguments --- init_size and max_size. >>> But since shared hash tables have fixed size there is little sense to >>> pass two arguments. In fact currently ShmemInitHash is always called >>> with init_size == max_size with only one exception, InitLocks procedure >>> (see lock.c), which I believe is actually a bug. >> No, I think we left it that way on purpose. I don't remember the >> discussion exactly, but I don't think it's hurting anything. > My instinct is telling me that this is useful in this shape for > plugins, and that it has been designed on purpose for that. Yes, it is that way on purpose. The comments for ShmemInitHash explain why: * init_size is the number of hashtable entries to preallocate. For a table * whose maximum size is certain, this should be equal to max_size; that * ensures that no run-time out-of-shared-memory failures can occur. The sizes of the lock hashtables are *not* certain, and in particular we're guessing as to the ratio of LOCK entries to PROCLOCK entries. By setting max_size larger than init_size, we leave a pool of initially-unallocated shared memory that can be doled out to either hashtable on demand, thus adapting to whatever the actual load is. (Of course, it'd be even better if such memory could be given back when the peak demand passes. But the lack of a full-on allocator for shared memory is not a reason to dumb down ShmemInitHash.) In short: the error in Aleksander's argument is the assumption that shared hashtables have fixed size. That's simply false. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small patch: Change calling convention for ShmemInitHash (and fix possible bug)
On Fri, Mar 25, 2016 at 9:17 PM, Robert Haas wrote: > On Thu, Mar 24, 2016 at 9:50 AM, Aleksander Alekseev > wrote: >> Currently this procedure has two arguments --- init_size and max_size. >> But since shared hash tables have fixed size there is little sense to >> pass two arguments. In fact currently ShmemInitHash is always called >> with init_size == max_size with only one exception, InitLocks procedure >> (see lock.c), which I believe is actually a bug. > > No, I think we left it that way on purpose. I don't remember the > discussion exactly, but I don't think it's hurting anything. My instinct is telling me that this is useful in this shape for plugins, and that it has been designed on purpose for that. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small patch: Change calling convention for ShmemInitHash (and fix possible bug)
> No, I think we left it that way on purpose. I don't remember the > discussion exactly, but I don't think it's hurting anything. This was a part of original dynahash optimization patch. Since that patch was about performance improvement and this concrete change is about refactoring, not performance, we agreed to discuss it later as a separate patch. > I don't think this actually buys us anything. For sure it doesn't make anything worse. Current code is just confusing. I spent quite some time trying to figure out what is a reason for passing two arguments before I realized - there is none. -- Best regards, Aleksander Alekseev http://eax.me/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small patch: Change calling convention for ShmemInitHash (and fix possible bug)
On Thu, Mar 24, 2016 at 9:50 AM, Aleksander Alekseev wrote: > I would like to continue discussion regarding changing calling > convention for ShmemInitHash procedure: > > http://www.postgresql.org/message-id/CA+TgmoZm=uowt8a_xasfoogwufeelj861ntadiceopyfehv...@mail.gmail.com > > Currently this procedure has two arguments --- init_size and max_size. > But since shared hash tables have fixed size there is little sense to > pass two arguments. In fact currently ShmemInitHash is always called > with init_size == max_size with only one exception, InitLocks procedure > (see lock.c), which I believe is actually a bug. No, I think we left it that way on purpose. I don't remember the discussion exactly, but I don't think it's hurting anything. > Patch is attached. > > What do you think? I don't think this actually buys us anything. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Small patch: Change calling convention for ShmemInitHash (and fix possible bug)
Hello
I would like to continue discussion regarding changing calling
convention for ShmemInitHash procedure:
http://www.postgresql.org/message-id/CA+TgmoZm=uowt8a_xasfoogwufeelj861ntadiceopyfehv...@mail.gmail.com
Currently this procedure has two arguments --- init_size and max_size.
But since shared hash tables have fixed size there is little sense to
pass two arguments. In fact currently ShmemInitHash is always called
with init_size == max_size with only one exception, InitLocks procedure
(see lock.c), which I believe is actually a bug.
Patch is attached.
What do you think?
--
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 3d9b8e4..4213c3a 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -495,7 +495,7 @@ pgss_shmem_startup(void)
info.hash = pgss_hash_fn;
info.match = pgss_match_fn;
pgss_hash = ShmemInitHash("pg_stat_statements hash",
- pgss_max, pgss_max,
+ pgss_max,
&info,
HASH_ELEM | HASH_FUNCTION | HASH_COMPARE);
diff --git a/src/backend/storage/buffer/buf_table.c b/src/backend/storage/buffer/buf_table.c
index 39e8baf..dd5acb7 100644
--- a/src/backend/storage/buffer/buf_table.c
+++ b/src/backend/storage/buffer/buf_table.c
@@ -62,7 +62,7 @@ InitBufTable(int size)
info.num_partitions = NUM_BUFFER_PARTITIONS;
SharedBufHash = ShmemInitHash("Shared Buffer Lookup Table",
- size, size,
+ size,
&info,
HASH_ELEM | HASH_BLOBS | HASH_PARTITION);
}
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index 81506ea..9df73b1 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -237,7 +237,7 @@ InitShmemIndex(void)
hash_flags = HASH_ELEM;
ShmemIndex = ShmemInitHash("ShmemIndex",
- SHMEM_INDEX_SIZE, SHMEM_INDEX_SIZE,
+ SHMEM_INDEX_SIZE,
&info, hash_flags);
}
@@ -250,14 +250,7 @@ InitShmemIndex(void)
* table at once. (In practice, all creations are done in the postmaster
* process; child processes should always be attaching to existing tables.)
*
- * max_size is the estimated maximum number of hashtable entries. This is
- * not a hard limit, but the access efficiency will degrade if it is
- * exceeded substantially (since it's used to compute directory size and
- * the hash table buckets will get overfull).
- *
- * init_size is the number of hashtable entries to preallocate. For a table
- * whose maximum size is certain, this should be equal to max_size; that
- * ensures that no run-time out-of-shared-memory failures can occur.
+ * max_size is the estimated maximum number of hashtable entries.
*
* Note: before Postgres 9.0, this function returned NULL for some failure
* cases. Now, it always throws error instead, so callers need not check
@@ -265,7 +258,6 @@ InitShmemIndex(void)
*/
HTAB *
ShmemInitHash(const char *name, /* table string name for shmem index */
- long init_size, /* initial table size */
long max_size, /* max size of the table */
HASHCTL *infoP, /* info about key and bucket size */
int hash_flags) /* info about infoP */
@@ -299,7 +291,7 @@ ShmemInitHash(const char *name, /* table string name for shmem index */
/* Pass location of hashtable header to hash_create */
infoP->hctl = (HASHHDR *) location;
- return hash_create(name, init_size, infoP, hash_flags);
+ return hash_create(name, max_size, infoP, hash_flags);
}
/*
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index b30b7b1..df41b29 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -377,8 +377,7 @@ void
InitLocks(void)
{
HASHCTL info;
- long init_table_size,
-max_table_size;
+ long max_table_size;
bool found;
/*
@@ -386,7 +385,6 @@ InitLocks(void)
* calculations must agree with LockShmemSize!
*/
max_table_size = NLOCKENTS();
- init_table_size = max_table_size / 2;
/*
* Allocate hash table for LOCK structs. This stores per-locked-object
@@ -398,14 +396,12 @@ InitLocks(void)
info.num_partitions = NUM_LOCK_PARTITIONS;
LockMethodLockHash = ShmemInitHash("LOCK hash",
- init_table_size,
max_table_size,
&info,
HASH_ELEM | HASH_BLOBS | HASH_PARTITION);
/* Assume an average of 2 holders per lock */
max_table_size *= 2;
- init_table_size *= 2;
/*
* Allocate hash table for PROCLOCK structs. This stores
@@ -417,7 +413,6 @@ InitLocks(void)
info.num_partitions = NUM_LOCK_PARTITIONS;
LockMethodProcLockHash = ShmemInitHash("PROCLOCK hash",
- init_table_size,
max_table_size,
&info,
HASH_ELEM | HASH_FUNCTION | HASH_PARTITION);
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 0
