public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v43 2/7] Add conditional lock feature to dshash
5+ messages / 3 participants
[nested] [flat]
* [PATCH v43 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.27.0
----Next_Part(Fri_Dec_11_16_50_03_2020_915)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v43-0003-Make-archiver-process-an-auxiliary-process.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Partitioned tables and [un]loggedness
@ 2024-08-29 14:49 Nathan Bossart <[email protected]>
2024-09-03 00:22 ` Re: Partitioned tables and [un]loggedness Michael Paquier <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Nathan Bossart @ 2024-08-29 14:49 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: David G. Johnston <[email protected]>; Postgres hackers <[email protected]>
On Thu, Aug 29, 2024 at 03:44:45PM +0900, Michael Paquier wrote:
> On Tue, Aug 27, 2024 at 04:01:58PM -0500, Nathan Bossart wrote:
>> My current thinking is that it would be better to disallow marking
>> partitioned tables as LOGGED/UNLOGGED and continue to have users explicitly
>> specify what they want for each partition. It'd still probably be good to
>> expand the documentation, but a clear ERROR when trying to set a
>> partitioned table as UNLOGGED would hopefully clue folks in.
>
> The addition of the new LOGGED keyword is not required if we limit
> ourselves to an error when defining UNLOGGED, so if we drop this
> proposal, let's also drop this part entirely and keep DefineRelation()
> simpler.
+1
> Actually, is really issuing an error the best thing we can
> do after so many years allowing this grammar flavor to go through,
> even if it is perhaps accidental? relpersistence is marked correctly
> for partitioned tables, it's just useless. Expanding the
> documentation sounds fine to me, one way or the other, to tell what
> happens with partitioned tables.
IMHO continuing to allow partitioned tables to be marked UNLOGGED just
preserves the illusion that it does something. An ERROR could help dispel
that misconception.
--
nathan
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Partitioned tables and [un]loggedness
2024-08-29 14:49 Re: Partitioned tables and [un]loggedness Nathan Bossart <[email protected]>
@ 2024-09-03 00:22 ` Michael Paquier <[email protected]>
2024-09-03 01:35 ` Re: Partitioned tables and [un]loggedness Nathan Bossart <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2024-09-03 00:22 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: David G. Johnston <[email protected]>; Postgres hackers <[email protected]>
On Thu, Aug 29, 2024 at 09:49:44AM -0500, Nathan Bossart wrote:
> IMHO continuing to allow partitioned tables to be marked UNLOGGED just
> preserves the illusion that it does something. An ERROR could help dispel
> that misconception.
Okay. This is going to be disruptive if we do nothing about pg_dump,
unfortunately. How about tweaking dumpTableSchema() so as we'd never
issue UNLOGGED for a partitioned table? We could filter that out as
there is tbinfo->relkind.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Partitioned tables and [un]loggedness
2024-08-29 14:49 Re: Partitioned tables and [un]loggedness Nathan Bossart <[email protected]>
2024-09-03 00:22 ` Re: Partitioned tables and [un]loggedness Michael Paquier <[email protected]>
@ 2024-09-03 01:35 ` Nathan Bossart <[email protected]>
2024-09-03 07:59 ` Re: Partitioned tables and [un]loggedness Michael Paquier <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Nathan Bossart @ 2024-09-03 01:35 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: David G. Johnston <[email protected]>; Postgres hackers <[email protected]>
On Tue, Sep 03, 2024 at 09:22:58AM +0900, Michael Paquier wrote:
> On Thu, Aug 29, 2024 at 09:49:44AM -0500, Nathan Bossart wrote:
>> IMHO continuing to allow partitioned tables to be marked UNLOGGED just
>> preserves the illusion that it does something. An ERROR could help dispel
>> that misconception.
>
> Okay. This is going to be disruptive if we do nothing about pg_dump,
> unfortunately. How about tweaking dumpTableSchema() so as we'd never
> issue UNLOGGED for a partitioned table? We could filter that out as
> there is tbinfo->relkind.
That's roughly what I had in mind.
--
nathan
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Partitioned tables and [un]loggedness
2024-08-29 14:49 Re: Partitioned tables and [un]loggedness Nathan Bossart <[email protected]>
2024-09-03 00:22 ` Re: Partitioned tables and [un]loggedness Michael Paquier <[email protected]>
2024-09-03 01:35 ` Re: Partitioned tables and [un]loggedness Nathan Bossart <[email protected]>
@ 2024-09-03 07:59 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2024-09-03 07:59 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: David G. Johnston <[email protected]>; Postgres hackers <[email protected]>
On Mon, Sep 02, 2024 at 08:35:15PM -0500, Nathan Bossart wrote:
> On Tue, Sep 03, 2024 at 09:22:58AM +0900, Michael Paquier wrote:
>> Okay. This is going to be disruptive if we do nothing about pg_dump,
>> unfortunately. How about tweaking dumpTableSchema() so as we'd never
>> issue UNLOGGED for a partitioned table? We could filter that out as
>> there is tbinfo->relkind.
>
> That's roughly what I had in mind.
An idea is attached. The pgbench bit was unexpected.
--
Michael
Attachments:
[text/x-diff] unlogged-part-v3.patch (4.0K, ../../[email protected]/2-unlogged-part-v3.patch)
download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b3cc6f8f69..b1cc635498 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -728,6 +728,12 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
else
partitioned = false;
+ if (relkind == RELKIND_PARTITIONED_TABLE &&
+ stmt->relation->relpersistence == RELPERSISTENCE_UNLOGGED)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("partitioned tables cannot be unlogged")));
+
/*
* Look up the namespace in which we are supposed to create the relation,
* check we have permission to create there, lock it against concurrent
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index dacb033e98..c60d2994f2 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15896,8 +15896,13 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
binary_upgrade_set_pg_class_oids(fout, q,
tbinfo->dobj.catId.oid);
+ /*
+ * PostgreSQL 18 has disabled UNLOGGED for partitioned tables, so
+ * ignore it when dumping if it was set in this case.
+ */
appendPQExpBuffer(q, "CREATE %s%s %s",
- tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
+ tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED &&
+ tbinfo->relkind != RELKIND_PARTITIONED_TABLE ?
"UNLOGGED " : "",
reltypename,
qualrelname);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 61618f2e18..dc4d7408cd 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4865,7 +4865,7 @@ initCreateTables(PGconn *con)
/* Construct new create table statement. */
printfPQExpBuffer(&query, "create%s table %s(%s)",
- unlogged_tables ? " unlogged" : "",
+ unlogged_tables && partition_method == PART_NONE ? " unlogged" : "",
ddl->table,
(scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 284a7fb85c..2654891524 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -50,6 +50,8 @@ ERROR: cannot create temporary relation in non-temporary schema
LINE 1: CREATE TEMP TABLE public.temp_to_perm (a int primary key);
^
DROP TABLE unlogged1, public.unlogged2;
+CREATE UNLOGGED TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- fail
+ERROR: partitioned tables cannot be unlogged
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
ERROR: relation "as_select1" already exists
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 1fd4cbfa7e..5dbb25a092 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -30,6 +30,8 @@ CREATE TEMP TABLE pg_temp.doubly_temp (a int primary key); -- also OK
CREATE TEMP TABLE public.temp_to_perm (a int primary key); -- not OK
DROP TABLE unlogged1, public.unlogged2;
+CREATE UNLOGGED TABLE unlogged1 (a int) PARTITION BY RANGE (a); -- fail
+
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 93b3f664f2..0859afd75e 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -220,6 +220,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
If this is specified, any sequences created together with the unlogged
table (for identity or serial columns) are also created as unlogged.
</para>
+
+ <para>
+ This option is not supported for partitioned tables.
+ </para>
</listitem>
</varlistentry>
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-09-03 07:59 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 v43 2/7] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]>
2024-08-29 14:49 Re: Partitioned tables and [un]loggedness Nathan Bossart <[email protected]>
2024-09-03 00:22 ` Re: Partitioned tables and [un]loggedness Michael Paquier <[email protected]>
2024-09-03 01:35 ` Re: Partitioned tables and [un]loggedness Nathan Bossart <[email protected]>
2024-09-03 07:59 ` Re: Partitioned tables and [un]loggedness Michael Paquier <[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