public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v39 2/7] Add conditional lock feature to dshash
5+ messages / 5 participants
[nested] [flat]
* [PATCH v39 2/7] Add conditional lock feature to dshash
@ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 5+ 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 b829167872..9c90096f3d 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 c337099061..493e974832 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.18.4
----Next_Part(Tue_Oct__6_10_06_44_2020_382)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v39-0003-Make-archiver-process-an-auxiliary-process.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add TOAST support for more system tables
@ 2023-07-17 22:31 Tom Lane <[email protected]>
2023-07-17 23:40 ` Re: Add TOAST support for more system tables David Rowley <[email protected]>
2023-07-18 05:32 ` Re: Add TOAST support for more system tables Michael Paquier <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Tom Lane @ 2023-07-17 22:31 UTC (permalink / raw)
To: Sofia Kopikova <[email protected]>; +Cc: pgsql-hackers
Sofia Kopikova <[email protected]> writes:
> This patch adds TOAST support for system tables pg_class,
> pg_attribute and pg_largeobject_metadata, as they include ACL columns,
> which may be potentially large in size.
We have been around on this topic before, cf discussion leading up to
commit 96cdeae07. Allowing toasted data in pg_class or pg_attribute
seems quite scary to me because of the potential for recursive access,
particularly during cache-flush scenarios. (That is, you need to be
able to read those catalogs on the way to fetching a toasted value,
so how can you be sure that doesn't devolve into an infinite loop?)
I wonder whether we'd be better off shoving the ACL data out of
these catalogs and putting it somewhere else (compare pg_attrdef).
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add TOAST support for more system tables
2023-07-17 22:31 Re: Add TOAST support for more system tables Tom Lane <[email protected]>
@ 2023-07-17 23:40 ` David Rowley <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: David Rowley @ 2023-07-17 23:40 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Sofia Kopikova <[email protected]>; pgsql-hackers
On Tue, 18 Jul 2023 at 10:31, Tom Lane <[email protected]> wrote:
> I wonder whether we'd be better off shoving the ACL data out of
> these catalogs and putting it somewhere else (compare pg_attrdef).
relpartbound is another column that could cause a pg_class row to grow
too large. I did have a patch [1] to move that column into
pg_partition. I imagine it's very bit rotted now.
David
[1] https://postgr.es/m/CAKJS1f9QjUwQrio20Pi%3DyCHmnouf4z3SfN8sqXaAcwREG6k0zQ%40mail.gmail.com
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add TOAST support for more system tables
2023-07-17 22:31 Re: Add TOAST support for more system tables Tom Lane <[email protected]>
@ 2023-07-18 05:32 ` Michael Paquier <[email protected]>
2023-07-18 11:19 ` Re: Add TOAST support for more system tables Sofia Kopikova <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2023-07-18 05:32 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Sofia Kopikova <[email protected]>; pgsql-hackers
On Mon, Jul 17, 2023 at 06:31:04PM -0400, Tom Lane wrote:
> Sofia Kopikova <[email protected]> writes:
> > This patch adds TOAST support for system tables pg_class,
> > pg_attribute and pg_largeobject_metadata, as they include ACL columns,
> > which may be potentially large in size.
>
> We have been around on this topic before, cf discussion leading up to
> commit 96cdeae07. Allowing toasted data in pg_class or pg_attribute
> seems quite scary to me because of the potential for recursive access,
> particularly during cache-flush scenarios. (That is, you need to be
> able to read those catalogs on the way to fetching a toasted value,
> so how can you be sure that doesn't devolve into an infinite loop?)
Yep. I have something to add here. The last time I poked at that, I
was wondering about two code paths that have specific comments on this
matter. Based on my notes:
1) finish_heap_swap() in cluster.c:
* pg_class doesn't have a toast relation, so we don't need to update the
* corresponding toast relation. Not that there's little point moving all
* relfrozenxid updates here since swap_relation_files() needs to write to
* pg_class for non-mapped relations anyway.
2) extract_autovac_opts() in autovacuum.c:
* we acquired the pg_class row. If pg_class had a TOAST table, this would
* be a risk; fortunately, it doesn't.
What has been posted makes zero adjustments in these areas.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add TOAST support for more system tables
2023-07-17 22:31 Re: Add TOAST support for more system tables Tom Lane <[email protected]>
2023-07-18 05:32 ` Re: Add TOAST support for more system tables Michael Paquier <[email protected]>
@ 2023-07-18 11:19 ` Sofia Kopikova <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Sofia Kopikova @ 2023-07-18 11:19 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers
On Mon, Jul 17, 2023 at 06:31:04PM -0400, Tom Lane wrote:
> Sofia Kopikova <[email protected]> writes:
>> This patch adds TOAST support for system tables pg_class,
>> pg_attribute and pg_largeobject_metadata, as they include ACL columns,
>> which may be potentially large in size.
> We have been around on this topic before, cf discussion leading up to
> commit 96cdeae07. Allowing toasted data in pg_class or pg_attribute
> seems quite scary to me because of the potential for recursive access,
> particularly during cache-flush scenarios. (That is, you need to be
> able to read those catalogs on the way to fetching a toasted value,
> so how can you be sure that doesn't devolve into an infinite loop?)
Many thanks for your reviews. I'm gonna do research and revise this
feature thoroughly.
I'll set status of the patch to "Waiting on author" for now.
--
Sofia Kopikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-07-18 11:19 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 07:58 [PATCH v39 2/7] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]>
2023-07-17 22:31 Re: Add TOAST support for more system tables Tom Lane <[email protected]>
2023-07-17 23:40 ` Re: Add TOAST support for more system tables David Rowley <[email protected]>
2023-07-18 05:32 ` Re: Add TOAST support for more system tables Michael Paquier <[email protected]>
2023-07-18 11:19 ` Re: Add TOAST support for more system tables Sofia Kopikova <[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