Hash table scans outside transactions

Started by Ashutosh Bapatalmost 3 years ago6 messages

ashutosh.bapat.oss@gmail.com

almost 3 years ago

Hi,
Hash table scans (seq_scan_table/level) are cleaned up at the end of a
transaction in AtEOXact_HashTables(). If a hash seq scan continues
beyond transaction end it will meet "ERROR: no hash_seq_search scan
for hash table" in deregister_seq_scan(). That seems like a limiting
the hash table usage.

Our use case is
1. Add/update/remove entries in hash table
2. Scan the existing entries and perform one transaction per entry
3. Close scan

repeat above steps in an infinite loop. Note that we do not
add/modify/delete entries in step 2. We can't use linked lists since
the entries need to be updated or deleted using hash keys. Because the
hash seq scan is cleaned up at the end of the transaction, we
encounter error in the 3rd step. I don't see that the actual hash
table scan depends upon the seq_scan_table/level[] which is cleaned up
at the end of the transaction.

I have following questions
1. Is there a way to avoid cleaning up seq_scan_table/level() when the
transaction ends?
2. Is there a way that we can use hash table implementation in
PostgreSQL code for our purpose?

--
Best Wishes,
Ashutosh Bapat

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

almost 3 years ago

In reply to: Ashutosh Bapat (#1)

Re: Hash table scans outside transactions

Bumping it to attract some attention.

On Tue, Mar 21, 2023 at 12:51 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Hi,
Hash table scans (seq_scan_table/level) are cleaned up at the end of a
transaction in AtEOXact_HashTables(). If a hash seq scan continues
beyond transaction end it will meet "ERROR: no hash_seq_search scan
for hash table" in deregister_seq_scan(). That seems like a limiting
the hash table usage.

Our use case is
1. Add/update/remove entries in hash table
2. Scan the existing entries and perform one transaction per entry
3. Close scan

repeat above steps in an infinite loop. Note that we do not
add/modify/delete entries in step 2. We can't use linked lists since
the entries need to be updated or deleted using hash keys. Because the
hash seq scan is cleaned up at the end of the transaction, we
encounter error in the 3rd step. I don't see that the actual hash
table scan depends upon the seq_scan_table/level[] which is cleaned up
at the end of the transaction.

I have following questions
1. Is there a way to avoid cleaning up seq_scan_table/level() when the
transaction ends?
2. Is there a way that we can use hash table implementation in
PostgreSQL code for our purpose?

--
Best Wishes,
Ashutosh Bapat

--
Best Wishes,
Ashutosh Bapat

Aidar Imamov

a.imamov@postgrespro.ru

8 months ago

In reply to: Ashutosh Bapat (#2)

2 attachment(s)

Re: Hash table scans outside transactions

Ashutosh Bapat wrote 2023-03-28 15:58:

Bumping it to attract some attention.

On Tue, Mar 21, 2023 at 12:51 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Hi,
Hash table scans (seq_scan_table/level) are cleaned up at the end of a
transaction in AtEOXact_HashTables(). If a hash seq scan continues
beyond transaction end it will meet "ERROR: no hash_seq_search scan
for hash table" in deregister_seq_scan(). That seems like a limiting
the hash table usage.

Our use case is
1. Add/update/remove entries in hash table
2. Scan the existing entries and perform one transaction per entry
3. Close scan

repeat above steps in an infinite loop. Note that we do not
add/modify/delete entries in step 2. We can't use linked lists since
the entries need to be updated or deleted using hash keys. Because the
hash seq scan is cleaned up at the end of the transaction, we
encounter error in the 3rd step. I don't see that the actual hash
table scan depends upon the seq_scan_table/level[] which is cleaned up
at the end of the transaction.

I have following questions
1. Is there a way to avoid cleaning up seq_scan_table/level() when the
transaction ends?
2. Is there a way that we can use hash table implementation in
PostgreSQL code for our purpose?

--
Best Wishes,
Ashutosh Bapat

Hi!
I tried to resend this thread directly to myself, but for some reason it
ended up in the whole hackers list..

I thought I'd chime in on this topic since it hasn't really been
discussed
anywhere else (or maybe I missed it).
I've attached two patches: the first one is a little test extension to
demonstrate the problem (just add "hash_test" to
"shared_preload_libraries"),
and the second is a possible solution. Basically, the idea is that we
don't
reset the scan counter if we find scans that started outside of the
current
transaction at the end. I have to admit, though, that I can't
immediately
say what other implications this might have or what else we need to
watch
out for if we try this.
Maybe any thoughts on that?

regards,
Aidar Imamov

Attachments:

exampl_ext.patchtext/x-diff; name=exampl_ext.patchDownload

diff --git a/contrib/Makefile b/contrib/Makefile
index 2f0a88d3f77..b4ff1391387 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -21,6 +21,7 @@ SUBDIRS = \
 		earthdistance	\
 		file_fdw	\
 		fuzzystrmatch	\
+		hash_test	\
 		hstore		\
 		intagg		\
 		intarray	\
diff --git a/contrib/hash_test/Makefile b/contrib/hash_test/Makefile
new file mode 100644
index 00000000000..a4ce6692127
--- /dev/null
+++ b/contrib/hash_test/Makefile
@@ -0,0 +1,18 @@
+# contrib/hash_test/Makefile
+
+MODULE_big = hash_test
+OBJS = \
+	hash_test.o
+
+PGFILEDESC = "hash_test - demostrate hash_seq_search and transaction issue"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/hash_test
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/hash_test/hash_test.c b/contrib/hash_test/hash_test.c
new file mode 100644
index 00000000000..7c9e24f40cc
--- /dev/null
+++ b/contrib/hash_test/hash_test.c
@@ -0,0 +1,73 @@
+
+#include "postgres.h"
+
+#include "access/xact.h"
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "postmaster/bgworker.h"
+#include "utils/hsearch.h"
+
+#define HASH_TEST_TABLE_NELEMS 2
+
+PG_MODULE_MAGIC;
+
+PGDLLEXPORT void hash_test_workhorse(Datum main_arg);
+
+static HTAB *hash_test_table;
+
+typedef struct
+{
+	/* key */
+	int			num_key;
+} hash_test_table_entry;
+
+void
+_PG_init(void)
+{
+	BackgroundWorker worker;
+
+	if (!process_shared_preload_libraries_in_progress)
+		return;
+
+	MemSet(&worker, 0, sizeof(BackgroundWorker));
+	worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
+		BGWORKER_BACKEND_DATABASE_CONNECTION;
+	worker.bgw_start_time = BgWorkerStart_ConsistentState;
+	worker.bgw_restart_time = BGW_NEVER_RESTART;
+	sprintf(worker.bgw_library_name, "hash_test");
+	sprintf(worker.bgw_function_name, "hash_test_workhorse");
+	sprintf(worker.bgw_name, "hash_test proccess");
+
+	RegisterBackgroundWorker(&worker);
+}
+
+void
+hash_test_workhorse(Datum main_arg)
+{
+	hash_test_table_entry *h_entry;
+	HASHCTL		ctl;
+	HASH_SEQ_STATUS hs;
+
+	BackgroundWorkerInitializeConnection(NULL, NULL, 0);
+
+	ctl.keysize = sizeof(int);
+	ctl.entrysize = sizeof(hash_test_table_entry);
+
+	hash_test_table = hash_create("hash_test_table",
+								  HASH_TEST_TABLE_NELEMS,
+								  &ctl,
+								  HASH_ELEM | HASH_BLOBS);
+
+	/* insert elements */
+	for (int i = 0; i < HASH_TEST_TABLE_NELEMS; i++)
+		hash_search(hash_test_table, &i, HASH_ENTER, NULL);
+
+	/* go through hash table */
+	hash_seq_init(&hs, hash_test_table);
+	while ((h_entry = hash_seq_search(&hs)) != NULL)
+	{
+		StartTransactionCommand();
+		/* do some stuff */
+		CommitTransactionCommand();
+	}
+}
diff --git a/contrib/hash_test/meson.build b/contrib/hash_test/meson.build
new file mode 100644
index 00000000000..bc3aac4e66d
--- /dev/null
+++ b/contrib/hash_test/meson.build
@@ -0,0 +1,10 @@
+
+hash_test_sources = files(
+  'hash_test.c',
+)
+
+hash_test = shared_module('hash_test',
+  hash_test_sources,
+  kwargs: contrib_mod_args,
+)
+contrib_targets += hash_test
diff --git a/contrib/meson.build b/contrib/meson.build
index ed30ee7d639..a2a67b1b3fd 100644
--- a/contrib/meson.build
+++ b/contrib/meson.build
@@ -29,6 +29,7 @@ subdir('dict_xsyn')
 subdir('earthdistance')
 subdir('file_fdw')
 subdir('fuzzystrmatch')
+subdir('hash_test')
 subdir('hstore')
 subdir('hstore_plperl')
 subdir('hstore_plpython')

mb_solution.patchtext/x-diff; name=mb_solution.patchDownload

diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 1ad155d446e..6c4b55ae759 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -1912,26 +1912,38 @@ has_seq_scans(HTAB *hashp)
 void
 AtEOXact_HashTables(bool isCommit)
 {
+	int			i;
+	bool		out_of_xact_scan = false;
+
 	/*
 	 * During abort cleanup, open scans are expected; just silently clean 'em
 	 * out.  An open scan at commit means someone forgot a hash_seq_term()
 	 * call, so complain.
 	 *
+	 * However, it should also be noted that at the end of a transaction, scans
+	 * that were opened outside of it may still be active. If this is the case,
+	 * we cannot simply reset the counter.
+	 *
 	 * Note: it's tempting to try to print the tabname here, but refrain for
 	 * fear of touching deallocated memory.  This isn't a user-facing message
 	 * anyway, so it needn't be pretty.
 	 */
-	if (isCommit)
+	for (i = 0; i < num_seq_scans; i++)
 	{
-		int			i;
-
-		for (i = 0; i < num_seq_scans; i++)
+		if (seq_scan_level[i] == 0)
 		{
-			elog(WARNING, "leaked hash_seq_search scan for hash table %p",
-				 seq_scan_tables[i]);
+			if (!isCommit)
+				return;
+
+			out_of_xact_scan = true;
 		}
+		else if (isCommit)
+			elog(WARNING, "leaked hash_seq_search scan for hash table %p",
+						  seq_scan_tables[i]);
 	}
-	num_seq_scans = 0;
+
+	if (!out_of_xact_scan)
+		num_seq_scans = 0;
 }
 
 /* Clean up any open scans at end of subtransaction */

Aidar Imamov

a.imamov@postgrespro.ru

8 months ago

In reply to: Aidar Imamov (#3)

Re: Hash table scans outside transactions

Aidar Imamov wrote 2025-05-21 00:32:

Ashutosh Bapat wrote 2023-03-28 15:58:

Bumping it to attract some attention.

On Tue, Mar 21, 2023 at 12:51 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Hi,
Hash table scans (seq_scan_table/level) are cleaned up at the end of
a
transaction in AtEOXact_HashTables(). If a hash seq scan continues
beyond transaction end it will meet "ERROR: no hash_seq_search scan
for hash table" in deregister_seq_scan(). That seems like a limiting
the hash table usage.

Our use case is
1. Add/update/remove entries in hash table
2. Scan the existing entries and perform one transaction per entry
3. Close scan

repeat above steps in an infinite loop. Note that we do not
add/modify/delete entries in step 2. We can't use linked lists since
the entries need to be updated or deleted using hash keys. Because
the
hash seq scan is cleaned up at the end of the transaction, we
encounter error in the 3rd step. I don't see that the actual hash
table scan depends upon the seq_scan_table/level[] which is cleaned
up
at the end of the transaction.

I have following questions
1. Is there a way to avoid cleaning up seq_scan_table/level() when
the
transaction ends?
2. Is there a way that we can use hash table implementation in
PostgreSQL code for our purpose?

--
Best Wishes,
Ashutosh Bapat

Hi!
I tried to resend this thread directly to myself, but for some reason
it
ended up in the whole hackers list..

I thought I'd chime in on this topic since it hasn't really been
discussed
anywhere else (or maybe I missed it).
I've attached two patches: the first one is a little test extension to
demonstrate the problem (just add "hash_test" to
"shared_preload_libraries"),
and the second is a possible solution. Basically, the idea is that we
don't
reset the scan counter if we find scans that started outside of the
current
transaction at the end. I have to admit, though, that I can't
immediately
say what other implications this might have or what else we need to
watch
out for if we try this.
Maybe any thoughts on that?

regards,
Aidar Imamov

Just bumping it again

regards,
Aidar Imamov

Dilip Kumar

dilipbalaut@gmail.com

8 months ago

In reply to: Aidar Imamov (#3)

Re: Hash table scans outside transactions

On Wed, May 21, 2025 at 3:02 AM Aidar Imamov <a.imamov@postgrespro.ru> wrote:

Hi!
I tried to resend this thread directly to myself, but for some reason it
ended up in the whole hackers list..

I thought I'd chime in on this topic since it hasn't really been
discussed
anywhere else (or maybe I missed it).
I've attached two patches: the first one is a little test extension to
demonstrate the problem (just add "hash_test" to
"shared_preload_libraries"),
and the second is a possible solution. Basically, the idea is that we
don't
reset the scan counter if we find scans that started outside of the
current
transaction at the end. I have to admit, though, that I can't
immediately
say what other implications this might have or what else we need to
watch
out for if we try this.
Maybe any thoughts on that?

I haven't reviewed the complete patch or tested it, but I don't see
any issues with it.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Tomas Vondra

tomas@vondra.me

8 months ago

In reply to: Dilip Kumar (#5)

Re: Hash table scans outside transactions

On 5/25/25 13:36, Dilip Kumar wrote:

On Wed, May 21, 2025 at 3:02 AM Aidar Imamov <a.imamov@postgrespro.ru> wrote:

Hi!
I tried to resend this thread directly to myself, but for some reason it
ended up in the whole hackers list..

I thought I'd chime in on this topic since it hasn't really been
discussed
anywhere else (or maybe I missed it).
I've attached two patches: the first one is a little test extension to
demonstrate the problem (just add "hash_test" to
"shared_preload_libraries"),
and the second is a possible solution. Basically, the idea is that we
don't
reset the scan counter if we find scans that started outside of the
current
transaction at the end. I have to admit, though, that I can't
immediately
say what other implications this might have or what else we need to
watch
out for if we try this.
Maybe any thoughts on that?

I haven't reviewed the complete patch or tested it, but I don't see
any issues with it.

I may be wrong, but I'd guess the restriction is there for a reason.
Maybe it's not needed anymore, or maybe it's needed only in some cases,
or something like that.

So the most important thing for a patch removing the restriction it is
to explain why it's there and why it's safe to relax it. The extension
demonstrating that the restriction exists doesn't really help with that,
it shows why we have it. Not hat it's safe to remove it.

I'm not a dynahash expert, but isn't the first sentence from dynahash.c
relevant:

* dynahash.c supports both local-to-a-backend hash tables and hash
* tables in shared memory. For shared hash tables, it is the caller's
* responsibility to provide appropriate access interlocking. The
* simplest convention is that a single LWLock protects the whole hash
* table. Searches (HASH_FIND or hash_seq_search) need only shared
* lock, but any update requires exclusive lock.

In other words, let's say you have a hash table, protected by LWLock.
That is, you're holding the lock in shared mode during seqscan. And then
we drop the locks for some reason (say, transaction end). The table can
be modified - won't that break the seqscan?

FWIW I think with the use case from the beginning of this thread:

1. Add/update/remove entries in hash table
2. Scan the existing entries and perform one transaction per entry
3. Close scan

Why not to simply build a linked list after step (1)?

regards

--
Tomas Vondra