public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v48 2/7] Add conditional lock feature to dshash 5+ messages / 4 participants [nested] [flat]
* [PATCH v48 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 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(Fri_Mar__5_17_18_56_2021_497)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v48-0003-Make-archiver-process-an-auxiliary-process.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Subscription sometimes loses txns after initial table sync @ 2024-12-09 13:20 Pritam Baral <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Pritam Baral @ 2024-12-09 13:20 UTC (permalink / raw) To: pgsql-hackers This was discovered when testing the plan for a major version upgrade via logical replication. Said plan requires that some tables be synced before others. So I implemented it using ALTER PUBLICATION ... ADD TABLE ... followed by ALTER SUBSCRIPTION ... REFRESH PUBLICATION. A test for correctness revealed that sometimes, for some tables added this way, txns after the initial data copy are lost by the subscription. A reproducer script is attached. It has been tested with PG 17.2, 14.15, and even 12.22 (on either side of the replication setup). The script runs at a default scale of 100 tables with 10k inserts each. This scale is enough to demonstrate a failure rate of 1% to 9% of tables on my modest laptop. In attempts to analyse why this happens, it has been observed that the sender sometimes does not pick up a published table, even when the receiver that started the sender process has seen the table as available (as returned by pg_get_publication_tables()) and has thus begun COPYing its data. When the COPY finishes (and the tablesync worker is finished), the apply loop on the receiver expects to receive (and apply) subsequent changes for such tables, but simply isn't sent any. This was observed by dumping every CopyData message sent over the wire. The attached script (like the original migration plan) uses a single publication and adds tables to it successively. Curiously, when the script was changed to use a dedicated publication per table (and thus, ALTER SUBSCRIPTION ... ADD PUBLICATION instead of ALTER SUBSCRIPTION ... REFRESH PUBLICATION), the no. of tables with data loss jumped to 100%. -- #!/usr/bin/env regards Chhatoi Pritam Baral Attachments: [application/x-shellscript] sub-loss-repro.sh (2.1K, ../../[email protected]/2-sub-loss-repro.sh) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Subscription sometimes loses txns after initial table sync @ 2024-12-09 17:57 Pritam Baral <[email protected]> parent: Pritam Baral <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Pritam Baral @ 2024-12-09 17:57 UTC (permalink / raw) To: pgsql-hackers On 09/12/24 18:50, Pritam Baral wrote: > A reproducer script is attached. Apologies. The aforementioned script is broken. It was a poor port from an internal application. A corrected reproducer script is attached. -- #!/usr/bin/env regards Chhatoi Pritam Baral Attachments: [application/x-shellscript] sub-loss-repro2.sh (3.1K, ../../[email protected]/3-sub-loss-repro2.sh) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* RE: Subscription sometimes loses txns after initial table sync @ 2024-12-10 01:53 Zhijie Hou (Fujitsu) <[email protected]> parent: Pritam Baral <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Zhijie Hou (Fujitsu) @ 2024-12-10 01:53 UTC (permalink / raw) To: Pritam Baral <[email protected]>; +Cc: pgsql-hackers On Monday, December 9, 2024 9:21 PM Pritam Baral <[email protected]> wrote: > To: pgsql-hackers <[email protected]> > Subject: Subscription sometimes loses txns after initial table sync > > This was discovered when testing the plan for a major version upgrade via > logical replication. Said plan requires that some tables be synced before > others. So I implemented it using ALTER PUBLICATION ... ADD TABLE ... > followed > by ALTER SUBSCRIPTION ... REFRESH PUBLICATION. A test for correctness > revealed > that sometimes, for some tables added this way, txns after the initial data copy > are lost by the subscription. > > A reproducer script is attached. It has been tested with PG 17.2, 14.15, and > even 12.22 (on either side of the replication setup). The script runs at a > default scale of 100 tables with 10k inserts each. This scale is enough to > demonstrate a failure rate of 1% to 9% of tables on my modest laptop. > > In attempts to analyse why this happens, it has been observed that the sender > sometimes does not pick up a published table, even when the receiver that > started the sender process has seen the table as available (as returned by > pg_get_publication_tables()) and has thus begun COPYing its data. When the > COPY > finishes (and the tablesync worker is finished), the apply loop on the receiver > expects to receive (and apply) subsequent changes for such tables, but simply > isn't sent any. This was observed by dumping every CopyData message sent > over > the wire. > > The attached script (like the original migration plan) uses a single publication > and adds tables to it successively. Curiously, when the script was changed to > use a dedicated publication per table (and thus, ALTER SUBSCRIPTION ... > ADD > PUBLICATION instead of ALTER SUBSCRIPTION ... REFRESH PUBLICATION), > the no. of > tables with data loss jumped to 100%. Thanks for reporting the issue. The described behavior looks similar to another bug discussed in [1]. If possible, could you please check if the latest patch in that thread can fix the bug you reported ? If it does, it would be helpful to share the feedback in that thread. [1] https://www.postgresql.org/message-id/flat/de52b282-1166-1180-45a2-8d8917ca74c6%40enterprisedb.com Best Regards, Hou zj ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Subscription sometimes loses txns after initial table sync @ 2024-12-11 04:50 Shlok Kyal <[email protected]> parent: Zhijie Hou (Fujitsu) <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Shlok Kyal @ 2024-12-11 04:50 UTC (permalink / raw) To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: Pritam Baral <[email protected]>; pgsql-hackers On Tue, 10 Dec 2024 at 07:24, Zhijie Hou (Fujitsu) <[email protected]> wrote: > > On Monday, December 9, 2024 9:21 PM Pritam Baral <[email protected]> wrote: > > To: pgsql-hackers <[email protected]> > > Subject: Subscription sometimes loses txns after initial table sync > > > > This was discovered when testing the plan for a major version upgrade via > > logical replication. Said plan requires that some tables be synced before > > others. So I implemented it using ALTER PUBLICATION ... ADD TABLE ... > > followed > > by ALTER SUBSCRIPTION ... REFRESH PUBLICATION. A test for correctness > > revealed > > that sometimes, for some tables added this way, txns after the initial data copy > > are lost by the subscription. > > > > A reproducer script is attached. It has been tested with PG 17.2, 14.15, and > > even 12.22 (on either side of the replication setup). The script runs at a > > default scale of 100 tables with 10k inserts each. This scale is enough to > > demonstrate a failure rate of 1% to 9% of tables on my modest laptop. > > > > In attempts to analyse why this happens, it has been observed that the sender > > sometimes does not pick up a published table, even when the receiver that > > started the sender process has seen the table as available (as returned by > > pg_get_publication_tables()) and has thus begun COPYing its data. When the > > COPY > > finishes (and the tablesync worker is finished), the apply loop on the receiver > > expects to receive (and apply) subsequent changes for such tables, but simply > > isn't sent any. This was observed by dumping every CopyData message sent > > over > > the wire. > > > > The attached script (like the original migration plan) uses a single publication > > and adds tables to it successively. Curiously, when the script was changed to > > use a dedicated publication per table (and thus, ALTER SUBSCRIPTION ... > > ADD > > PUBLICATION instead of ALTER SUBSCRIPTION ... REFRESH PUBLICATION), > > the no. of > > tables with data loss jumped to 100%. > > Thanks for reporting the issue. > > The described behavior looks similar to another bug discussed in [1]. If > possible, could you please check if the latest patch in that thread can fix the > bug you reported ? > > If it does, it would be helpful to share the feedback in that thread. > > [1] https://www.postgresql.org/message-id/flat/de52b282-1166-1180-45a2-8d8917ca74c6%40enterprisedb.com > Hi, I tried to reproduce the issue on HEAD and REL_17_STABLE branches. I found that the issue is intermittent for me. I ran the script, provided in [1], 50 times on both branches and I was able to reproduce the issue 4 times and 5 times respectively. Then I tested both the branches after applying patches in [2] and ran the script 50 times. I was not able to reproduce the issue with patch. I think as Hou-san suggested, the patches in [2] can fix this issue. [1]: https://www.postgresql.org/message-id/8b595156-d8b6-4b53-a788-7d945726cd2f%40pritambaral.com [2]: https://www.postgresql.org/message-id/flat/de52b282-1166-1180-45a2-8d8917ca74c6%40enterprisedb.com Thanks and Regards, Shlok Kyal ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-12-11 04:50 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 v48 2/7] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]> 2024-12-09 13:20 Subscription sometimes loses txns after initial table sync Pritam Baral <[email protected]> 2024-12-09 17:57 ` Re: Subscription sometimes loses txns after initial table sync Pritam Baral <[email protected]> 2024-12-10 01:53 ` RE: Subscription sometimes loses txns after initial table sync Zhijie Hou (Fujitsu) <[email protected]> 2024-12-11 04:50 ` Re: Subscription sometimes loses txns after initial table sync Shlok Kyal <[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