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]> 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-09-03 00:22 Michael Paquier <[email protected]> parent: 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-09-03 01:35 Nathan Bossart <[email protected]> parent: 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-09-03 07:59 Michael Paquier <[email protected]> parent: Nathan Bossart <[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