Shared memory hash table #1379

levkk · 2023-11-07T22:38:27Z

Shared memory hash table. Implementation based (pretty much copied from) on pg_stat_statements. Subject to the same shared memory limitations as the rest of shared memory data structures, but still cool to have a native Postgres HashMap implementation in shared memory.

levkk · 2023-11-08T17:22:37Z

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x9)
    frame #0: 0x00000001a00f7b38 libsystem_platform.dylib`_platform_strcmp + 8
libsystem_platform.dylib`_platform_strcmp:
->  0x1a00f7b38 <+8>:  ldrb   w4, [x0], #0x1
    0x1a00f7b3c <+12>: ldrb   w5, [x1], #0x1
    0x1a00f7b40 <+16>: subs   x3, x4, x5
    0x1a00f7b44 <+20>: ccmp   w4, #0x0, #0x4, eq
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x9)
  * frame #0: 0x00000001a00f7b38 libsystem_platform.dylib`_platform_strcmp + 8
    frame #1: 0x000000019ffcd310 libsystem_c.dylib`hash_search + 116
    frame #2: 0x000000010191dfbc shmem.so`{closure#0} at pg13.rs:27674:1
    frame #3: 0x00000001019079f0 shmem.so`hash_search [inlined] pg_guard_ffi_boundary_impl<*mut core::ffi::c_void, pgrx_pg_sys::include::pg13::hash_search::{closure_env#0}> at ffi.rs:120:26
    frame #4: 0x0000000101907930 shmem.so`hash_search [inlined] pg_guard_ffi_boundary<*mut core::ffi::c_void, pgrx_pg_sys::include::pg13::hash_search::{closure_env#0}> at ffi.rs:94:14
    frame #5: 0x0000000101907930 shmem.so`hash_search at pg13.rs:27674:1
    frame #6: 0x00000001018ef51c shmem.so`insert at shmem_hash.rs:59:13
    frame #7: 0x00000001018a3d4c shmem.so`hash_table_insert at lib.rs:68:5
    frame #8: 0x00000001018a3dd4 shmem.so`hash_table_insert_wrapper_inner at lib.rs:67:1
    frame #9: 0x00000001018cc810 shmem.so`{closure#0} at lib.rs:67:1
    frame #10: 0x00000001018d6e0c shmem.so`do_call<shmem::hash_table_insert_wrapper::{closure_env#0}, ()> at panicking.rs:500:40
    frame #11: 0x00000001018d7b14 shmem.so`__rust_try + 32
    frame #12: 0x00000001018d6ab8 shmem.so`try<(), shmem::hash_table_insert_wrapper::{closure_env#0}> at panicking.rs:464:19
    frame #13: 0x00000001018aa644 shmem.so`catch_unwind<shmem::hash_table_insert_wrapper::{closure_env#0}, ()> at panic.rs:142:14
    frame #14: 0x00000001018bcdd0 shmem.so`run_guarded<shmem::hash_table_insert_wrapper::{closure_env#0}, ()> at panic.rs:408:11
    frame #15: 0x00000001018bff10 shmem.so`pgrx_extern_c_guard<shmem::hash_table_insert_wrapper::{closure_env#0}, ()> at panic.rs:385:11
    frame #16: 0x00000001018a3d84 shmem.so`hash_table_insert_wrapper at lib.rs:67:1
    frame #17: 0x0000000100d174b4 postgres`ExecInterpExpr(state=0x0000000145018970, econtext=<unavailable>, isnull=<unavailable>) at execExprInterp.c:704:8 [opt]
    frame #18: 0x0000000100d4a7d8 postgres`ExecResult [inlined] ExecEvalExprSwitchContext(state=0x0000000145018970, econtext=0x0000000145018670, isNull=0x000000016f275e4f) at executor.h:322:13 [opt]
    frame #19: 0x0000000100d4a7b0 postgres`ExecResult [inlined] ExecProject(projInfo=0x0000000145018968) at executor.h:356:9 [opt]
    frame #20: 0x0000000100d4a794 postgres`ExecResult(pstate=<unavailable>) at nodeResult.c:136:10 [opt]
    frame #21: 0x0000000100d1e3d0 postgres`standard_ExecutorRun [inlined] ExecProcNode(node=0x0000000145018558) at executor.h:248:9 [opt]
    frame #22: 0x0000000100d1e3b4 postgres`standard_ExecutorRun [inlined] ExecutePlan(estate=0x0000000145018320, planstate=0x0000000145018558, use_parallel_mode=<unavailable>, operation=CMD_SELECT, sendTuples=<unavailable>, numberTuples=0, direction=<unavailable>, dest=0x0000000135009158, execute_once=<unavailable>) at execMain.c:1632:10 [opt]
    frame #23: 0x0000000100d1e388 postgres`standard_ExecutorRun(queryDesc=0x000000014500eb20, direction=<unavailable>, count=0, execute_once=<unavailable>) at execMain.c:350:3 [opt]
    frame #24: 0x0000000100d1e2c0 postgres`ExecutorRun(queryDesc=<unavailable>, direction=<unavailable>, count=<unavailable>, execute_once=<unavailable>) at execMain.c:294:3 [opt] [artificial]
    frame #25: 0x0000000100e9847c postgres`PortalRunSelect(portal=0x000000014682d920, forward=<unavailable>, count=0, dest=0x0000000135009158) at pquery.c:921:4 [opt]
    frame #26: 0x0000000100e980b4 postgres`PortalRun(portal=0x000000014682d920, count=9223372036854775807, isTopLevel=<unavailable>, run_once=<unavailable>, dest=0x0000000135009158, altdest=0x0000000135009158, qc=0x000000016f2760e8) at pquery.c:765:18 [opt]
    frame #27: 0x0000000100e972e4 postgres`exec_simple_query(query_string="select hash_table_insert(10, 9);") at postgres.c:1238:10 [opt]
    frame #28: 0x0000000100e94808 postgres`PostgresMain(argc=<unavailable>, argv=<unavailable>, dbname=<unavailable>, username=<unavailable>) at postgres.c:0 [opt]
    frame #29: 0x0000000100e11500 postgres`BackendRun(port=0x0000000144f047e0) at postmaster.c:4557:2 [opt]
    frame #30: 0x0000000100e10c20 postgres`ServerLoop [inlined] BackendStartup(port=0x0000000144f047e0) at postmaster.c:4241:3 [opt]
    frame #31: 0x0000000100e10bfc postgres`ServerLoop at postmaster.c:1739:7 [opt]
    frame #32: 0x0000000100e0dff8 postgres`PostmasterMain(argc=<unavailable>, argv=<unavailable>) at postmaster.c:1412:11 [opt]
    frame #33: 0x0000000100d69b34 postgres`main(argc=8, argv=0x0000600001b30dc0) at main.c:210:3 [opt]
    frame #34: 0x000000019fd73f28 dyld`start + 2236
(lldb)

levkk · 2023-11-08T17:25:33Z

@thomcc Hi Thom, I'm getting a segfault here and the backtrace (comment above) is showing that the linker picked up the wrong hash_search function. Instead of using the one in postgres, it's using the one in Mac OS's libc. Any ideas?

To repro:

cargo pgrx run the shmem example
SELECT pg_backend_pid()
sudo lldb -p <backend pid>
SELECT hash_table_insert(10, 9) twice. The first time, it'll be ok, the second you should get a segfault. Basically, calling the wrong hash_search function twice results in bad memory access.

Thank you!

pgrx/src/shmem_hash.rs

workingjubilee · 2023-11-09T18:47:10Z

  Error: bindgen failed for pg16

  Caused by:
     0: Unable to generate bindings for pg16
     1: clang diagnosed error: /usr/lib/llvm-14/lib/clang/14.0.0/include/emmintrin.h:2228:10: error: invalid conversion between vector type '__m128i' (vector of 2 'long long' values) and integer type 'int' of different size
        /usr/lib/llvm-14/lib/clang/14.0.0/include/emmintrin.h:2250:10: error: invalid conversion between vector type '__m128i' (vector of 2 'long long' values) and integer type 'int' of different size
        /usr/lib/llvm-14/lib/clang/14.0.0/include/emmintrin.h:2271:10: error: invalid conversion between vector type '__m128i' (vector of 2 'long long' values) and integer type 'int' of different size
        /usr/lib/llvm-14/lib/clang/14.0.0/include/emmintrin.h:2292:10: error: invalid conversion between vector type '__m128i' (vector of 2 'long long' values) and integer type 'int' of different size
        /usr/lib/llvm-14/lib/clang/14.0.0/include/emmintrin.h:2670:10: error: invalid conversion between vector type '__m128i' (vector of 2 'long long' values) and integer type 'int' of different size
        /usr/lib/llvm-14/lib/clang/14.0.0/include/emmintrin.h:2691:10: error: invalid conversion between vector type '__m128i' (vector of 2 'long long' values) and integer type 'int' of different size
        /usr/lib/llvm-14/lib/clang/14.0.0/include/emmintrin.h:2711:10: error: invalid conversion between vector type '__m128i' (vector of 2 'long long' values) and integer type 'int' of different size
        /usr/lib/llvm-14/lib/clang/14.0.0/include/emmintrin.h:2731:10: error: invalid conversion between vector type '__m128i' (vector of 2 'long long' values) and integer type 'int' of different size


  Location:
      pgrx-pg-sys/build.rs:733:10

not this shit again.

levkk · 2023-11-09T19:57:03Z

Yeah I also lied about this being infinitely large...still hitting out of memory when allocating more than 512kb or so. Gonna keep debugging to find out why. Probably some shared memory limit constant somewhere?

workingjubilee · 2023-11-09T20:02:52Z

we got #1351 over that, it's weird.

workingjubilee · 2023-11-09T20:28:00Z

Replace PgLwLock with PgSpinLock. However, PgSpinLock constructor isn't const, not sure why. I don't see it used anywhere, so might not be ready yet?

Please feel free to make PgSpinLock::new a const fn.

levkk · 2023-11-09T21:03:11Z

Replace PgLwLock with PgSpinLock. However, PgSpinLock constructor isn't const, not sure why. I don't see it used anywhere, so might not be ready yet?

Please feel free to make PgSpinLock::new a const fn.

Will do!

levkk · 2023-11-17T03:05:20Z

Yes, I should finish this...

workingjubilee · 2023-11-17T06:44:14Z

I just kicked all the open PRs for CI reasons, don't worry~

levkk · 2023-11-27T18:15:05Z

Looks like cargo fmt in develop needs to be run.

workingjubilee · 2023-11-27T18:30:42Z

Whoops.
But also

111 | pub use shmem_hash::*;

levkk · 2023-11-27T18:37:35Z

Ah forgot a cfg. One moment.

levkk · 2023-11-27T18:41:09Z

Look at that build queue! $$$

levkk · 2023-11-27T19:03:35Z

Now I feel bad! Maybe we could have just one job sniff testing basics before launching the whole test suite?

workingjubilee · 2023-11-27T18:58:13Z

pgrx/src/shmem_hash.rs

+
+#[derive(Debug, Eq, PartialEq)]
+#[non_exhaustive]
+pub enum Error {


A public error type shall be given a name that does not collide with the name of the Error trait.

workingjubilee · 2023-11-27T19:00:06Z

pgrx/src/shmem_hash.rs

+/// A shared memory HashMap using Postgres' `HTAB`.
+/// This HashMap is used for `pg_stat_statements` and Postgres
+/// internals to store key/value pairs in shared memory.
+pub struct PgHashMap<K: Copy + Clone, V: Copy + Clone> {


The type for new APIs shall not be prefixed with "Pg". It's Postgres, everything's Pg everywhere, all these types are namespaced via pgrx::, and this style is inconsistently used at best (Array vs. PgHeapTuple, for instance).

ShmemHashMap would be fine.

workingjubilee · 2023-11-27T19:04:08Z

pgrx/src/shmem_hash.rs

+impl<K: Copy + Clone, V: Copy + Clone> PgHashMap<K, V> {
+    /// Create new `PgHashMap`. This still needs to be allocated with
+    /// `pg_shmem_init!` just like any other shared memory structure.
+    pub const fn new(size: u64) -> PgHashMap<K, V> {


It's common for ::new() to be semantically equivalent to ::default(), so the size parameter's purpose should be documented rather than left implicit, if we require it.

It should also be made clear why it's a u64 instead of the more common usize. Yes, the difference "doesn't matter", but the difference between those two types informs what purpose it serves.

workingjubilee · 2023-11-27T19:09:14Z

pgrx/src/spinlock.rs

@@ -28,6 +28,7 @@ use std::{cell::UnsafeCell, marker::PhantomData};
 /// [`storage/spin.h`]:
 ///     https://github.com/postgres/postgres/blob/1f0c4fa255253d223447c2383ad2b384a6f05854/src/include/storage/spin.h
 #[doc(alias = "slock_t")]
+#[derive(Debug)]


This will just say

PgSpinLock { item: UnsafeCell { .. }, lock: UnsafeCell { .. } }

Please impl Debug explicitly with something more concise.

workingjubilee · 2023-11-27T20:05:58Z

Now I feel bad! Maybe we could have just one job sniff testing basics before launching the whole test suite?

I certainly would not reject PRs improving our test suite's economy of CPU cycles!

workingjubilee

Thank you. I realize reviewing twice on entirely different parts of the PR like this may be slightly unusual(?), and I appreciate you bearing with me here. My first pass often is about smaller nits because once they are addressed, it's easier to focus on the finer details.

workingjubilee · 2023-11-27T20:53:24Z

pgrx/src/shmem_hash.rs

+    /// * `size` - Maximum number of elements in the HashMap. This is allocated
+    ///            at server start and cannot be changed. `i64` is the expected type
+    ///            for `pg_sys::ShmemInitHash`, so we don't attempt runtime conversions
+    ///            unnecessarily.


...okay, "you only can allocate this once ever" is pretty coherent as to why we need it, yeah. Though I believe a runtime conversion that happens once ever is... fine?

workingjubilee · 2023-11-27T20:54:54Z

pgrx/src/shmem_hash.rs

    /// Get the number of elements in the HashMap.
-    pub fn len(&self) -> u64 {
+    pub fn len(&self) -> i64 {


This reminds me, this needs capacity(&self) as well.

workingjubilee · 2023-11-27T21:01:38Z

pgrx/src/shmem_hash.rs

+    /// Remove the value from the `ShmemHashMap` and return it.
+    /// If the key doesn't exist, return None.
+    pub fn remove(&self, key: K) -> Option<V> {
+        if let Some(value) = self.get(key) {


This should probably use let-else, like so:

Suggested change

if let Some(value) = self.get(key) {

let Some(value) = self.get(key) else { return None };

workingjubilee · 2023-11-27T21:03:11Z

pgrx/src/shmem_hash.rs

+        } else {
+            let value_ptr = value_ptr!(entry);
+            let value = unsafe { std::ptr::read(value_ptr) };
+            return Some(value.value);


Suggested change

return Some(value.value);

Some(value.value)

workingjubilee · 2023-11-27T21:03:17Z

pgrx/src/shmem_hash.rs

+        };
+
+        if entry.is_null() {
+            return None;


Suggested change

return None;

None

workingjubilee · 2023-11-27T21:10:13Z

pgrx/src/shmem_hash.rs

+            return Some(value);
+        } else {
+            return None;
+        }


But if you do use let-else this should probably be updated.

Something like this, but then it will want you to hit it with rustfmt.

Suggested change

return Some(value);

} else {

return None;

}

Some(value)

workingjubilee · 2023-11-27T21:18:08Z

pgrx/src/shmem_hash.rs

+            let value_ptr = value_ptr!(entry);
+            let value = unsafe { std::ptr::read(value_ptr) };
+            return Some(value.value);


Is there a reason this returns V and not &V? Because we cannot rely on concurrent changes not altering the data in the K/V pair? It would be good to understand the soundness conditions better here and why things look the way they do.

workingjubilee · 2023-11-27T21:20:44Z

pgrx/src/shmem_hash.rs

+/// A shared memory HashMap using Postgres' `HTAB`.
+/// This HashMap is used for `pg_stat_statements` and Postgres
+/// internals to store key/value pairs in shared memory.
+pub struct ShmemHashMap<K: Copy + Clone, V: Copy + Clone> {


Please weaken the bounds for the struct and instead keep them on the implementation. It should be made clear why certain functions require the bounds they do in order to be sound, instead of plastering the bounds across the entire API. Aside from leaking implementation details, it makes it unclear where the soundness requirements actually start, which means later refactoring that attempts to weaken the bounds can be made unsound.

Note that it's fine if the bounds are overeagerly applied in cases where it's not clear, but e.g. I have done a lot of this refactoring on Array<'_, T> and List<T> recently, and in general I tried to pull the scope tighter.

workingjubilee · 2023-11-27T21:25:41Z

pgrx/src/shmem_hash.rs

+        let htab_ptr = unsafe {
+            pg_sys::ShmemInitHash(


Preconditions, postconditions, "why is this sound?", etc.

workingjubilee · 2023-11-27T21:30:16Z

pgrx/src/shmem_hash.rs

+    /// HTAB protected by a SpinLock.
+    htab: OnceCell<PgSpinLock<ShmemHashMapInner>>,


Will the use of OnceCell remain correct in the event of this being fixed?

PgLwLock breaks postgres reinitialization after backend crash #1223

It's fine if the answer is "no", I just want to check to make sure if this should get a note relating it back to that issue.

workingjubilee · 2023-11-27T21:36:01Z

pgrx-tests/src/tests/shmem_tests.rs

@@ -11,22 +11,35 @@ use pgrx::prelude::*;
 use pgrx::{pg_shmem_init, PgAtomic, PgLwLock, PgSharedMemoryInitialization};
 use std::sync::atomic::AtomicBool;

+#[cfg(feature = "cshim")]
+use pgrx::PgHashMap;


Suggested change

use pgrx::PgHashMap;

use pgrx::ShmemHashMap;

workingjubilee · 2023-11-27T21:36:17Z

pgrx-tests/src/tests/shmem_tests.rs

 static ATOMIC: PgAtomic<AtomicBool> = PgAtomic::new();
 static LWLOCK: PgLwLock<bool> = PgLwLock::new();

+#[cfg(feature = "cshim")]
+static HASH_MAP: PgHashMap<i64, i64> = PgHashMap::new(500);


Suggested change

static HASH_MAP: PgHashMap<i64, i64> = PgHashMap::new(500);

static HASH_MAP: ShmemHashMap<i64, i64> = ShmemHashMap::new(500);

levkk added 3 commits November 7, 2023 11:46

htab

926201c

hmmm

afb25ab

wah

31ddf43

clean

edd3300

levkk added 3 commits November 8, 2023 15:50

alright!

f47615a

alright!

b69a6dc

comment

a168537

levkk changed the title ~~[WIP] Shared memory hash table~~ Shared memory hash table Nov 8, 2023

levkk marked this pull request as ready for review November 9, 2023 00:03

kczimm reviewed Nov 9, 2023

View reviewed changes

pgrx/src/shmem_hash.rs Outdated Show resolved Hide resolved

tests

8ccbd74

levkk marked this pull request as draft November 9, 2023 17:13

levkk added 2 commits November 9, 2023 09:49

alright!

6e088de

remove

f490b99

workingjubilee closed this Nov 17, 2023

workingjubilee reopened this Nov 17, 2023

levkk added 2 commits November 18, 2023 10:01

finish up hashmap

fe95ef3

comment

cd9d6cc

levkk marked this pull request as ready for review November 18, 2023 18:10

workingjubilee closed this Nov 27, 2023

workingjubilee reopened this Nov 27, 2023

Feature flag

34760d2

Almost!

87558bc

workingjubilee reviewed Nov 27, 2023

View reviewed changes

Address comments

e077414

workingjubilee reviewed Nov 27, 2023

View reviewed changes

workingjubilee requested changes Nov 27, 2023

View reviewed changes

	if let Some(value) = self.get(key) {
	let Some(value) = self.get(key) else { return None };

		/// HTAB protected by a SpinLock.
		htab: OnceCell<PgSpinLock<ShmemHashMapInner>>,

	static HASH_MAP: PgHashMap<i64, i64> = PgHashMap::new(500);
	static HASH_MAP: ShmemHashMap<i64, i64> = ShmemHashMap::new(500);

Shared memory hash table #1379

Are you sure you want to change the base?

Shared memory hash table #1379

Conversation

levkk commented Nov 7, 2023 • edited Loading

levkk commented Nov 8, 2023

levkk commented Nov 8, 2023

workingjubilee commented Nov 9, 2023

levkk commented Nov 9, 2023

workingjubilee commented Nov 9, 2023

workingjubilee commented Nov 9, 2023

levkk commented Nov 9, 2023

levkk commented Nov 17, 2023

workingjubilee commented Nov 17, 2023

levkk commented Nov 27, 2023

workingjubilee commented Nov 27, 2023

levkk commented Nov 27, 2023

levkk commented Nov 27, 2023

levkk commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Nov 27, 2023

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levkk commented Nov 7, 2023 •

edited

Loading