public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v47 2/7] Add conditional lock feature to dshash
7+ messages / 4 participants
[nested] [flat]
* [PATCH v47 2/7] Add conditional lock feature to dshash
@ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 7+ 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(Thu_Jan_21_12_03_48_2021_284)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v47-0003-Make-archiver-process-an-auxiliary-process.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* pg_upgrade-breaking release
@ 2025-04-24 12:11 Bruce Momjian <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Bruce Momjian @ 2025-04-24 12:11 UTC (permalink / raw)
To: PostgreSQL-development <[email protected]>
Do we think most people are _not_ going to use pg_upgrade now that we
are defaulting to checksums being enabled by default in PG 18? And if
so, do we think we are ever going to have a storage-format-changing
release where pg_upgrade cannot be used?
Maybe it is too late to ask this, but I am asking anyway.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Do not let urgent matters crowd out time for investment in the future.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_upgrade-breaking release
@ 2025-04-24 12:35 Greg Sabino Mullane <[email protected]>
parent: Bruce Momjian <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Greg Sabino Mullane @ 2025-04-24 12:35 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: PostgreSQL-development <[email protected]>
On Thu, Apr 24, 2025 at 8:12 AM Bruce Momjian <[email protected]> wrote:
> Do we think most people are _not_ going to use pg_upgrade now that we
> are defaulting to checksums being enabled by default in PG 18?
I cannot imagine this would stop anyone from upgrading. It's one additional
flag, which was already a requirement for those going the other direction
in the past (checksums -> no checksums). And I also assume that "most
people" are already running with checksums enabled.
And if so, do we think we are ever going to have a
> storage-format-changing release where pg_upgrade cannot be used?
>
Seems very unlikely, that would kind of go against the whole purpose of
pg_upgrade.
--
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_upgrade-breaking release
@ 2025-04-24 12:37 Bruce Momjian <[email protected]>
parent: Greg Sabino Mullane <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Bruce Momjian @ 2025-04-24 12:37 UTC (permalink / raw)
To: Greg Sabino Mullane <[email protected]>; +Cc: PostgreSQL-development <[email protected]>
On Thu, Apr 24, 2025 at 08:35:10AM -0400, Greg Sabino Mullane wrote:
> On Thu, Apr 24, 2025 at 8:12 AM Bruce Momjian <[email protected]> wrote:
>
> Do we think most people are _not_ going to use pg_upgrade now that we
> are defaulting to checksums being enabled by default in PG 18?
>
>
> I cannot imagine this would stop anyone from upgrading. It's one additional
> flag, which was already a requirement for those going the other direction in
> the past (checksums -> no checksums). And I also assume that "most people" are
> already running with checksums enabled.
>
>
> And if so, do we think we are ever going to have a
> storage-format-changing release where pg_upgrade cannot be used?
>
>
> Seems very unlikely, that would kind of go against the whole purpose of
> pg_upgrade.
When I wrote pg_upgrade, I assumed at some point the value of changing
the storage format would outweigh the value of allowing in-place
upgrades. I guess that hasn't happened yet.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Do not let urgent matters crowd out time for investment in the future.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_upgrade-breaking release
@ 2025-04-24 12:51 Greg Sabino Mullane <[email protected]>
parent: Bruce Momjian <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Greg Sabino Mullane @ 2025-04-24 12:51 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: PostgreSQL-development <[email protected]>
On Thu, Apr 24, 2025 at 8:37 AM Bruce Momjian <[email protected]> wrote:
> When I wrote pg_upgrade, I assumed at some point the value of changing the
> storage format would outweigh the value of allowing in-place upgrades. I
> guess that hasn't happened yet.
>
It reminds me of TDE, which is an good example of that, in a way. It
requires pg_dump or logical replication to go from unencrypted ->
encrypted. If core were to have something like that, I guess pg_upgrade
could technically support it, but it would be equivalent to pg_dump. We've
all been spoiled by that awesome --link option! :)
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_upgrade-breaking release
@ 2025-04-24 12:58 Bruce Momjian <[email protected]>
parent: Greg Sabino Mullane <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Bruce Momjian @ 2025-04-24 12:58 UTC (permalink / raw)
To: Greg Sabino Mullane <[email protected]>; +Cc: PostgreSQL-development <[email protected]>
On Thu, Apr 24, 2025 at 08:51:41AM -0400, Greg Sabino Mullane wrote:
> On Thu, Apr 24, 2025 at 8:37 AM Bruce Momjian <[email protected]> wrote:
>
> When I wrote pg_upgrade, I assumed at some point the value of changing the
> storage format would outweigh the value of allowing in-place upgrades. I
> guess that hasn't happened yet.
>
>
> It reminds me of TDE, which is an good example of that, in a way. It requires
> pg_dump or logical replication to go from unencrypted -> encrypted. If core
> were to have something like that, I guess pg_upgrade could technically support
> it, but it would be equivalent to pg_dump. We've all been spoiled by that
> awesome --link option! :)
True on spoiled, but I think TDE is being held back by code complexity,
not upgrade complexity.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Do not let urgent matters crowd out time for investment in the future.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_upgrade-breaking release
@ 2025-04-24 14:08 Noah Misch <[email protected]>
parent: Bruce Momjian <[email protected]>
1 sibling, 0 replies; 7+ messages in thread
From: Noah Misch @ 2025-04-24 14:08 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; PostgreSQL-development <[email protected]>
On Thu, Apr 24, 2025 at 08:37:56AM -0400, Bruce Momjian wrote:
> On Thu, Apr 24, 2025 at 08:35:10AM -0400, Greg Sabino Mullane wrote:
> > On Thu, Apr 24, 2025 at 8:12 AM Bruce Momjian <[email protected]> wrote:
> >
> > Do we think most people are _not_ going to use pg_upgrade now that we
> > are defaulting to checksums being enabled by default in PG 18?
> >
> >
> > I cannot imagine this would stop anyone from upgrading. It's one additional
> > flag
Yeah. We've had this before, with integer datetimes and others. People will
notice the friction, but v18 won't be a shock in this respect.
> > And if so, do we think we are ever going to have a
> > storage-format-changing release where pg_upgrade cannot be used?
> >
> >
> > Seems very unlikely, that would kind of go against the whole purpose of
> > pg_upgrade.
>
> When I wrote pg_upgrade, I assumed at some point the value of changing
> the storage format would outweigh the value of allowing in-place
> upgrades. I guess that hasn't happened yet.
Having pg_upgrade has made PostgreSQL popular for applications with high
availability and high data volumes, applications that would have ruled out
PostgreSQL before pg_upgrade. In other words, adding pg_upgrade changed the
expectations of PostgreSQL. A storage format break is less plausible now than
it was in the early years of having pg_upgrade.
I think a prerequisite for a storage format break would be a foolproof upgrade
recipe based on logical replication techniques. The step having downtime
would need to be no slower than pg_upgrade. Even with that, the double
storage requirement isn't negligible. Hence, it wouldn't be a given that we'd
regard the storage format break as sufficiently-valuable.
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2025-04-24 14:08 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 07:58 [PATCH v47 2/7] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]>
2025-04-24 12:11 pg_upgrade-breaking release Bruce Momjian <[email protected]>
2025-04-24 12:35 ` Re: pg_upgrade-breaking release Greg Sabino Mullane <[email protected]>
2025-04-24 12:37 ` Re: pg_upgrade-breaking release Bruce Momjian <[email protected]>
2025-04-24 12:51 ` Re: pg_upgrade-breaking release Greg Sabino Mullane <[email protected]>
2025-04-24 12:58 ` Re: pg_upgrade-breaking release Bruce Momjian <[email protected]>
2025-04-24 14:08 ` Re: pg_upgrade-breaking release Noah Misch <[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