Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support shared_preload_libraries etc... #45

Open
jamessewell opened this issue May 22, 2019 · 10 comments
Open

Support shared_preload_libraries etc... #45

jamessewell opened this issue May 22, 2019 · 10 comments

Comments

@jamessewell
Copy link

Hi,

Just wanted to know how you'd feel about adding support for shared_preload_libraries / bgworkers \ shared memory (although that's a lot harder) being added?

@bluejekyll
Copy link
Owner

I’m a big fan of us using this library for extending Postgres in as many different ways as possible.

I’m currently a little concerned about the memory model for a lot of this, but yes, I’m a big supporter.

@jamessewell
Copy link
Author

jamessewell commented May 22, 2019 via email

@bluejekyll
Copy link
Owner

bluejekyll commented May 22, 2019

I’m working on a patch to try and make things threadsafe in regards to the use of palloc. I can reproduce issues with it right now, but don’t have it isolated to what the exact problem is, yet.

@jamessewell
Copy link
Author

jamessewell commented May 22, 2019 via email

@bluejekyll
Copy link
Owner

I hang out in the Rust discord app, we can chat there.

This is all in the context of the shared library loading, as currently implemented. Threading would be from the Rust, though it might be in appropriate in general in Postgres, but I expect people coming from Rust will want to use threading...

@jamessewell
Copy link
Author

jamessewell commented Aug 29, 2020

Just saw this issue I'd forgotten and thought I'd comment with the current state of play - turns out calling Postgres through FFI (as this crate does) is unsafe even in the presence of threads.

ie: if you use Rayon, or something which starts a thread to do work in the background then you are firmly into the realms of UB - even if the threads never call into Postgres.

@bluejekyll
Copy link
Owner

I agree that threads with PG FFI are inherently dangerous, given the nature of PG.

I think an option we have is to pass around a & mut instance of the memory context, and that would not be sendable across threads. If we required that handle on all FFI, then it would be possible to reduce mistakes in that context.

Not sure it’s worth it though. BG workers are probably the safe and best model here.

@jamessewell
Copy link
Author

jamessewell commented Aug 30, 2020 via email

@bluejekyll
Copy link
Owner

bluejekyll commented Aug 30, 2020

So we already mark PgDatum and PgAllocated types as both !Send and !Sync specifically to deny sharing memory managed by PG between threads. Additionally, the PgAllocator is !Send and !Sync.

Otherwise we use the default Rust allocator for all memory. Is there a reason you think that isn't safe? I'm not honestly sure we have any changes necessary with those restrictions in place.

The FFI calls are still dangerous, I agree about that though.

@jamessewell
Copy link
Author

jamessewell commented Aug 31, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants