public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v52 2/7] Add conditional lock feature to dshash 2+ messages / 2 participants [nested] [flat]
* [PATCH v52 2/7] Add conditional lock feature to dshash @ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Kyotaro Horiguchi @ 2020-03-13 07:58 UTC (permalink / raw) Dshash currently waits for lock unconditionally. It is inconvenient when we want to avoid being blocked by other processes. This commit adds alternative functions of dshash_find and dshash_find_or_insert that allows immediate return on lock failure. --- src/backend/lib/dshash.c | 98 +++++++++++++++++++++------------------- src/include/lib/dshash.h | 3 ++ 2 files changed, 55 insertions(+), 46 deletions(-) diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c index 520bfa0979..853d78b528 100644 --- a/src/backend/lib/dshash.c +++ b/src/backend/lib/dshash.c @@ -383,6 +383,10 @@ dshash_get_hash_table_handle(dshash_table *hash_table) * the caller must take care to ensure that the entry is not left corrupted. * The lock mode is either shared or exclusive depending on 'exclusive'. * + * If found is not NULL, *found is set to true if the key is found in the hash + * table. If the key is not found, *found is set to false and a pointer to a + * newly created entry is returned. + * * The caller must not lock a lock already. * * Note that the lock held is in fact an LWLock, so interrupts will be held on @@ -392,36 +396,7 @@ dshash_get_hash_table_handle(dshash_table *hash_table) void * dshash_find(dshash_table *hash_table, const void *key, bool exclusive) { - dshash_hash hash; - size_t partition; - dshash_table_item *item; - - hash = hash_key(hash_table, key); - partition = PARTITION_FOR_HASH(hash); - - Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); - - LWLockAcquire(PARTITION_LOCK(hash_table, partition), - exclusive ? LW_EXCLUSIVE : LW_SHARED); - ensure_valid_bucket_pointers(hash_table); - - /* Search the active bucket. */ - item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash)); - - if (!item) - { - /* Not found. */ - LWLockRelease(PARTITION_LOCK(hash_table, partition)); - return NULL; - } - else - { - /* The caller will free the lock by calling dshash_release_lock. */ - hash_table->find_locked = true; - hash_table->find_exclusively_locked = exclusive; - return ENTRY_FROM_ITEM(item); - } + return dshash_find_extended(hash_table, key, exclusive, false, false, NULL); } /* @@ -439,31 +414,60 @@ dshash_find_or_insert(dshash_table *hash_table, const void *key, bool *found) { - dshash_hash hash; - size_t partition_index; - dshash_partition *partition; + return dshash_find_extended(hash_table, key, true, false, true, found); +} + + +/* + * Find the key in the hash table. + * + * "exclusive" is the lock mode in which the partition for the returned item + * is locked. If "nowait" is true, the function immediately returns if + * required lock was not acquired. "insert" indicates insert mode. In this + * mode new entry is inserted and set *found to false. *found is set to true if + * found. "found" must be non-null in this mode. + */ +void * +dshash_find_extended(dshash_table *hash_table, const void *key, + bool exclusive, bool nowait, bool insert, bool *found) +{ + dshash_hash hash = hash_key(hash_table, key); + size_t partidx = PARTITION_FOR_HASH(hash); + dshash_partition *partition = &hash_table->control->partitions[partidx]; + LWLockMode lockmode = exclusive ? LW_EXCLUSIVE : LW_SHARED; dshash_table_item *item; - hash = hash_key(hash_table, key); - partition_index = PARTITION_FOR_HASH(hash); - partition = &hash_table->control->partitions[partition_index]; - - Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); + /* must be exclusive when insert allowed */ + Assert(!insert || (exclusive && found != NULL)); restart: - LWLockAcquire(PARTITION_LOCK(hash_table, partition_index), - LW_EXCLUSIVE); + if (!nowait) + LWLockAcquire(PARTITION_LOCK(hash_table, partidx), lockmode); + else if (!LWLockConditionalAcquire(PARTITION_LOCK(hash_table, partidx), + lockmode)) + return NULL; + ensure_valid_bucket_pointers(hash_table); /* Search the active bucket. */ item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash)); if (item) - *found = true; + { + if (found) + *found = true; + } else { - *found = false; + if (found) + *found = false; + + if (!insert) + { + /* The caller didn't told to add a new entry. */ + LWLockRelease(PARTITION_LOCK(hash_table, partidx)); + return NULL; + } /* Check if we are getting too full. */ if (partition->count > MAX_COUNT_PER_PARTITION(hash_table)) @@ -479,7 +483,8 @@ restart: * Give up our existing lock first, because resizing needs to * reacquire all the locks in the right order to avoid deadlocks. */ - LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); + LWLockRelease(PARTITION_LOCK(hash_table, partidx)); + resize(hash_table, hash_table->size_log2 + 1); goto restart; @@ -493,12 +498,13 @@ restart: ++partition->count; } - /* The caller must release the lock with dshash_release_lock. */ + /* The caller will free the lock by calling dshash_release_lock. */ hash_table->find_locked = true; - hash_table->find_exclusively_locked = true; + hash_table->find_exclusively_locked = exclusive; return ENTRY_FROM_ITEM(item); } + /* * Remove an entry by key. Returns true if the key was found and the * corresponding entry was removed. diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h index a6ea377173..5b8114d041 100644 --- a/src/include/lib/dshash.h +++ b/src/include/lib/dshash.h @@ -91,6 +91,9 @@ extern void *dshash_find(dshash_table *hash_table, const void *key, bool exclusive); extern void *dshash_find_or_insert(dshash_table *hash_table, const void *key, bool *found); +extern void *dshash_find_extended(dshash_table *hash_table, const void *key, + bool exclusive, bool nowait, bool insert, + bool *found); extern bool dshash_delete_key(dshash_table *hash_table, const void *key); extern void dshash_delete_entry(dshash_table *hash_table, void *entry); extern void dshash_release_lock(dshash_table *hash_table, void *entry); -- 2.27.0 ----Next_Part(Wed_Mar_10_17_51_37_2021_192)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v52-0003-Make-archiver-process-an-auxiliary-process.patch" ^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: Add index scan progress to pg_stat_progress_vacuum @ 2022-01-06 20:41 Bossart, Nathan <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Bossart, Nathan @ 2022-01-06 20:41 UTC (permalink / raw) To: Imseih (AWS), Sami <[email protected]>; Justin Pryzby <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; Peter Geoghegan <[email protected]>; pgsql-hackers On 12/29/21, 8:44 AM, "Imseih (AWS), Sami" <[email protected]> wrote: > In "pg_stat_progress_vacuum", introduce 2 columns: > > * total_index_vacuum : This is the # of indexes that will be vacuumed. Keep in mind that if failsafe mode kicks in mid-flight to the vacuum, Postgres may choose to forgo index scans. This value will be adjusted accordingly. > * max_index_vacuum_cycle_time : The total elapsed time for a index vacuum cycle is calculated and this value will be updated to reflect the longest vacuum cycle. Until the first cycle completes, this value will be 0. The purpose of this column is to give the user an idea of how long an index vacuum cycle takes to complete. I think that total_index_vacuum is a good thing to have. I would expect this to usually just be the number of indexes on the table, but as you pointed out, this can be different when we are skipping indexes. My only concern with this new column is the potential for confusion when compared with the index_vacuum_count value. index_vacuum_count indicates the number of vacuum cycles completed, but total_index_vacuum indicates the number of indexes that will be vacuumed. However, the names sound like they could refer to the same thing to me. Perhaps we should rename index_vacuum_count to index_vacuum_cycles/index_vacuum_cycle_count, and the new column should be something like num_indexes_to_vacuum or index_vacuum_total. I don't think we need the max_index_vacuum_cycle_time column. While the idea is to give users a rough estimate for how long an index cycle will take, I don't think it will help generate any meaningful estimates for how much longer the vacuum operation will take. IIUC we won't have any idea how many total index vacuum cycles will be needed. Even if we did, the current cycle could take much more or much less time. Also, none of the other progress views seem to provide any timing information, which I suspect is by design to avoid inaccurate estimates. > Introduce a new view called "pg_stat_progress_vacuum_index". This view will track the progress of a worker ( or leader PID ) while it's vacuuming an index. It will expose some key columns: > > * pid: The PID of the worker process > > * leader_pid: The PID of the leader process. This is the column that can be joined with "pg_stat_progress_vacuum". leader_pid and pid can have the same value as a leader can also perform an index vacuum. > > * indrelid: The relid of the index currently being vacuumed > > * vacuum_cycle_ordinal_position: The processing position of the index being vacuumed. This can be useful to determine how many indexes out of the total indexes ( pg_stat_progress_vacuum.total_index_vacuum ) have been vacuumed > > * index_tuples_vacuumed: This is the number of index tuples vacuumed for the index overall. This is useful to show that the vacuum is actually doing work, as the # of tuples keeps increasing. Should we also provide some information for determining the progress of the current cycle? Perhaps there should be an index_tuples_vacuumed_current_cycle column that users can compare with the num_dead_tuples value in pg_stat_progress_vacuum. However, perhaps the number of tuples vacuumed in the current cycle can already be discovered via index_tuples_vacuumed % max_dead_tuples. +void +rusage_adjust(const PGRUsage *ru0, PGRUsage *ru1) +{ + if (ru1->tv.tv_usec < ru0->tv.tv_usec) + { + ru1->tv.tv_sec--; + ru1->tv.tv_usec += 1000000; + } + if (ru1->ru.ru_stime.tv_usec < ru0->ru.ru_stime.tv_usec) + { + ru1->ru.ru_stime.tv_sec--; + ru1->ru.ru_stime.tv_usec += 1000000; + } + if (ru1->ru.ru_utime.tv_usec < ru0->ru.ru_utime.tv_usec) + { + ru1->ru.ru_utime.tv_sec--; + ru1->ru.ru_utime.tv_usec += 1000000; + } +} I think this function could benefit from a comment. Without going through it line by line, it is not clear to me exactly what it is doing. I know we're still working on what exactly this stuff should look like, but I would suggest adding the documentation changes in the near future. Nathan ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2022-01-06 20:41 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 07:58 [PATCH v52 2/7] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]> 2022-01-06 20:41 Re: Add index scan progress to pg_stat_progress_vacuum Bossart, Nathan <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox