introduce #[pg_guard(unsafe_entry_thread)] (pgcentralfoundation#2242)

initial commit of rejiggering how pgrx tracks the "ACTIVE_THREAD". This
is to address issue pgcentralfoundation#2228.

Functions annotated with `#[pg_guard(unsafe_entry_thread)]` do some
trickery to reset pgrx' understanding of the "ACTIVE_THREAD" so that
whatever thread happens to be calling that function then becomes our
understanding of the "ACTIVE_THREAD", thereby allowing pgrx to do
Postgres FFI on a thread that isn't the main thread.

The situation behind pgcentralfoundation#2228 is that pg_duckdb, an extension written in
C++, uses threads (presumably "safely") within Postgres and if one of
those threads does something like SPI, which then causes
planner/executor hooks to be called, and one of those hooks is
implemented with pgrx, this new `unsafe_entry_thread` tag will let that
callback run. It's basically making an assumption that the caller is
properly managing the thread it's on.

There's very little code changes here -- the bulk of the PR is a new
example crate/extension with its own little c-shim so that I could
manually test Postgres itself kicking off a thread that does SPI that
ends up calling into a planner hook setup by the rust/pgrx extension.

I have a need to push this through for $DayJob, so I'm probably going to
do that later this week.

---------

Co-authored-by: Eric B. Ridge <[email protected]>

Author: eeeebbbbrrrr

Cargo.toml

@@ -36,6 +36,7 @@ exclude = [
     "pgrx-examples/wal_decoder",
     "pgrx-examples/errors",
     "pgrx-examples/hooks",
+    "pgrx-examples/pgthread",
     "pgrx-examples/nostd",
     "pgrx-examples/numeric",
     "pgrx-examples/pgtrybuilder",

pgrx-bindgen/src/build.rs

@@ -7,13 +7,11 @@
 //LICENSE All rights reserved.
 //LICENSE
 //LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file.
+use crate::{detect_pg_config, env_tracked, is_for_release};
 use bindgen::NonCopyUnionStyle;
 use bindgen::callbacks::{DeriveTrait, EnumVariantValue, ImplementsTrait, MacroParsingBehavior};
 use eyre::{WrapErr, eyre};
-use pgrx_pg_config::{
-    PgConfig, PgConfigSelector, PgMinorVersion, PgVersion, Pgrx, SUPPORTED_VERSIONS,
-    is_supported_major_version,
-};
+use pgrx_pg_config::{PgConfig, PgMinorVersion, PgVersion, Pgrx};
 use quote::{ToTokens, quote};
 use std::cell::RefCell;
 use std::cmp::Ordering;
@@ -161,81 +159,13 @@ pub fn main() -> eyre::Result<()> {
     }
 
     let compile_cshim = env_tracked("CARGO_FEATURE_CSHIM").as_deref() == Some("1");
-    let is_for_release =
-        env_tracked("PGRX_PG_SYS_GENERATE_BINDINGS_FOR_RELEASE").as_deref() == Some("1");
-
     let build_paths = BuildPaths::from_env();
 
     eprintln!("build_paths={build_paths:?}");
 
     emit_rerun_if_changed();
 
-    let pg_configs: Vec<(u16, PgConfig)> = if is_for_release {
-        // This does not cross-check config.toml and Cargo.toml versions, as it is release infra.
-        Pgrx::from_config()?.iter(PgConfigSelector::All)
-            .map(|r| r.expect("invalid pg_config"))
-            .map(|c| (c.major_version().expect("invalid major version"), c))
-            .filter_map(|t| {
-                if is_supported_major_version(t.0) {
-                    Some(t)
-                } else {
-                    println!(
-                        "cargo:warning={} contains a configuration for pg{}, which pgrx does not support.",
-                        Pgrx::config_toml()
-                            .expect("Could not get PGRX configuration TOML")
-                            .to_string_lossy(),
-                        t.0
-                    );
-                    None
-                }
-            })
-            .collect()
-    } else {
-        let mut found = Vec::new();
-        for pgver in SUPPORTED_VERSIONS() {
-            if env_tracked(&format!("CARGO_FEATURE_PG{}", pgver.major)).is_some() {
-                found.push(pgver);
-            }
-        }
-        let found_ver = match &found[..] {
-            [ver] => ver,
-            [] => {
-                return Err(eyre!(
-                    "Did not find `pg$VERSION` feature. `pgrx-pg-sys` requires one of {} to be set",
-                    SUPPORTED_VERSIONS()
-                        .iter()
-                        .map(|pgver| format!("`pg{}`", pgver.major))
-                        .collect::<Vec<_>>()
-                        .join(", ")
-                ));
-            }
-            versions => {
-                return Err(eyre!(
-                    "Multiple `pg$VERSION` features found.\n`--no-default-features` may be required.\nFound: {}",
-                    versions
-                        .iter()
-                        .map(|version| format!("pg{}", version.major))
-                        .collect::<Vec<String>>()
-                        .join(", ")
-                ));
-            }
-        };
-
-        let found_major = found_ver.major;
-        if let Ok(pg_config) = PgConfig::from_env() {
-            let major_version = pg_config.major_version()?;
-
-            if major_version != found_major {
-                panic!(
-                    "Feature flag `pg{found_major}` does not match version from the environment-described PgConfig (`{major_version}`)"
-                )
-            }
-            vec![(major_version, pg_config)]
-        } else {
-            let specific = Pgrx::from_config()?.get(&format!("pg{}", found_ver.major))?;
-            vec![(found_ver.major, specific)]
-        }
-    };
+    let pg_configs = detect_pg_config()?;
 
     // make sure we're not trying to build any of the yanked postgres versions
     for (_, pg_config) in &pg_configs {
@@ -262,7 +192,7 @@ pub fn main() -> eyre::Result<()> {
                         *pg_major_ver,
                         pg_config,
                         &build_paths,
-                        is_for_release,
+                        is_for_release(),
                         compile_cshim,
                     )
                 })
@@ -903,37 +833,6 @@ fn add_derives(bind: bindgen::Builder) -> bindgen::Builder {
         .derive_partialord(false)
 }
 
-fn env_tracked(s: &str) -> Option<String> {
-    // a **sorted** list of environment variable keys that cargo might set that we don't need to track
-    // these were picked out, by hand, from: https://doc.rust-lang.org/cargo/reference/environment-variables.html
-    const CARGO_KEYS: &[&str] = &[
-        "BROWSER",
-        "DEBUG",
-        "DOCS_RS",
-        "HOST",
-        "HTTP_PROXY",
-        "HTTP_TIMEOUT",
-        "NUM_JOBS",
-        "OPT_LEVEL",
-        "OUT_DIR",
-        "PATH",
-        "PROFILE",
-        "TARGET",
-        "TERM",
-    ];
-
-    let is_cargo_key =
-        s.starts_with("CARGO") || s.starts_with("RUST") || CARGO_KEYS.binary_search(&s).is_ok();
-
-    if !is_cargo_key {
-        // if it's an envar that cargo gives us, we don't want to ask it to rerun build.rs if it changes
-        // we'll let cargo figure that out for itself, and doing so, depending on the key, seems to
-        // cause cargo to rerun build.rs every time, which is terrible
-        println!("cargo:rerun-if-env-changed={s}");
-    }
-    std::env::var(s).ok()
-}
-
 fn target_env_tracked(s: &str) -> Option<String> {
     let target = env_tracked("TARGET").unwrap();
     env_tracked(&format!("{s}_{target}")).or_else(|| env_tracked(s))

pgrx-bindgen/src/lib.rs

@@ -1 +1,122 @@
+//LICENSE Portions Copyright 2019-2021 ZomboDB, LLC.
+//LICENSE
+//LICENSE Portions Copyright 2021-2023 Technology Concepts & Design, Inc.
+//LICENSE
+//LICENSE Portions Copyright 2023-2023 PgCentral Foundation, Inc. <[email protected]>
+//LICENSE
+//LICENSE All rights reserved.
+//LICENSE
+//LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file.
+use eyre::eyre;
+use pgrx_pg_config::{
+    PgConfig, PgConfigSelector, Pgrx, SUPPORTED_VERSIONS, is_supported_major_version,
+};
+
 pub mod build;
+
+fn is_for_release() -> bool {
+    env_tracked("PGRX_PG_SYS_GENERATE_BINDINGS_FOR_RELEASE").as_deref() == Some("1")
+}
+
+/// Determines which `pg_config` is to be used, based on a combination of pgrx' internal knowledge
+/// of supported Postgres versions and `cargo` options/feature flags.
+pub fn detect_pg_config() -> eyre::Result<Vec<(u16, PgConfig)>> {
+    let pg_configs: Vec<(u16, PgConfig)> = if is_for_release() {
+        // This does not cross-check config.toml and Cargo.toml versions, as it is release infra.
+        Pgrx::from_config()?.iter(PgConfigSelector::All)
+            .map(|r| r.expect("invalid pg_config"))
+            .map(|c| (c.major_version().expect("invalid major version"), c))
+            .filter_map(|t| {
+                if is_supported_major_version(t.0) {
+                    Some(t)
+                } else {
+                    println!(
+                        "cargo:warning={} contains a configuration for pg{}, which pgrx does not support.",
+                        Pgrx::config_toml()
+                            .expect("Could not get PGRX configuration TOML")
+                            .to_string_lossy(),
+                        t.0
+                    );
+                    None
+                }
+            })
+            .collect()
+    } else {
+        let mut found = Vec::new();
+        for pgver in SUPPORTED_VERSIONS() {
+            if env_tracked(&format!("CARGO_FEATURE_PG{}", pgver.major)).is_some() {
+                found.push(pgver);
+            }
+        }
+        let found_ver = match &found[..] {
+            [ver] => ver,
+            [] => {
+                return Err(eyre!(
+                    "Did not find `pg$VERSION` feature. `pgrx-pg-sys` requires one of {} to be set",
+                    SUPPORTED_VERSIONS()
+                        .iter()
+                        .map(|pgver| format!("`pg{}`", pgver.major))
+                        .collect::<Vec<_>>()
+                        .join(", ")
+                ));
+            }
+            versions => {
+                return Err(eyre!(
+                    "Multiple `pg$VERSION` features found.\n`--no-default-features` may be required.\nFound: {}",
+                    versions
+                        .iter()
+                        .map(|version| format!("pg{}", version.major))
+                        .collect::<Vec<String>>()
+                        .join(", ")
+                ));
+            }
+        };
+
+        let found_major = found_ver.major;
+        if let Ok(pg_config) = PgConfig::from_env() {
+            let major_version = pg_config.major_version()?;
+
+            if major_version != found_major {
+                panic!(
+                    "Feature flag `pg{found_major}` does not match version from the environment-described PgConfig (`{major_version}`)"
+                )
+            }
+            vec![(major_version, pg_config)]
+        } else {
+            let specific = Pgrx::from_config()?.get(&format!("pg{}", found_ver.major))?;
+            vec![(found_ver.major, specific)]
+        }
+    };
+    Ok(pg_configs)
+}
+
+fn env_tracked(s: &str) -> Option<String> {
+    // a **sorted** list of environment variable keys that cargo might set that we don't need to track
+    // these were picked out, by hand, from: https://doc.rust-lang.org/cargo/reference/environment-variables.html
+    const CARGO_KEYS: &[&str] = &[
+        "BROWSER",
+        "DEBUG",
+        "DOCS_RS",
+        "HOST",
+        "HTTP_PROXY",
+        "HTTP_TIMEOUT",
+        "NUM_JOBS",
+        "OPT_LEVEL",
+        "OUT_DIR",
+        "PATH",
+        "PROFILE",
+        "TARGET",
+        "TERM",
+    ];
+
+    let is_cargo_key =
+        s.starts_with("CARGO") || s.starts_with("RUST") || CARGO_KEYS.binary_search(&s).is_ok();
+
+    if !is_cargo_key {
+        // if it's an envar that cargo gives us, we don't want to ask it to rerun build.rs if it changes
+        // we'll let cargo figure that out for itself, and doing so, depending on the key, seems to
+        // cause cargo to rerun build.rs every time, which is terrible
+        println!("cargo:rerun-if-env-changed={s}");
+    }
+    std::env::var(s).ok()
+}

pgrx-examples/pgthread/.gitignore

@@ -0,0 +1,10 @@
+.DS_Store
+.idea/
+/target
+*.iml
+**/*.rs.bk
+Cargo.lock
+sql/*
+c_ext/*.o
+c_ext/*.dylib
+c_ext/*.so

pgrx-examples/pgthread/Cargo.toml

@@ -0,0 +1,51 @@
+#LICENSE Portions Copyright 2019-2021 ZomboDB, LLC.
+#LICENSE
+#LICENSE Portions Copyright 2021-2023 Technology Concepts & Design, Inc.
+#LICENSE
+#LICENSE Portions Copyright 2023-2023 PgCentral Foundation, Inc. <[email protected]>
+#LICENSE
+#LICENSE All rights reserved.
+#LICENSE
+#LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file.
+
+[package]
+name = "pgthread"
+version = "0.0.0"
+edition = "2021"
+publish = false
+
+[lib]
+crate-type = ["cdylib", "lib"]
+
+[[bin]]
+name = "pgrx_embed_pgthread"
+path = "./src/bin/pgrx_embed.rs"
+
+[features]
+default = ["pg13"]
+pg13 = ["pgrx/pg13", "pgrx-tests/pg13" ]
+pg14 = ["pgrx/pg14", "pgrx-tests/pg14" ]
+pg15 = ["pgrx/pg15", "pgrx-tests/pg15" ]
+pg16 = ["pgrx/pg16", "pgrx-tests/pg16" ]
+pg17 = ["pgrx/pg17", "pgrx-tests/pg17" ]
+pg18 = ["pgrx/pg18", "pgrx-tests/pg18" ]
+pg_test = []
+
+[dependencies]
+pgrx = { path = "../../pgrx", default-features = false }
+
+[dev-dependencies]
+pgrx-tests = { path = "../../pgrx-tests" }
+
+[build-dependencies]
+pgrx-bindgen = { path = "../../pgrx-bindgen" }
+
+# uncomment these if compiling outside of 'pgrx'
+# [profile.dev]
+# panic = "unwind"
+
+# [profile.release]
+# panic = "unwind"
+# opt-level = 3
+# lto = "fat"
+# codegen-units = 1

pgrx-examples/pgthread/build.rs

@@ -0,0 +1,57 @@
+use std::process::{Command, ExitCode};
+
+use std::fs;
+use std::path::PathBuf;
+
+fn main() -> Result<ExitCode, Box<dyn std::error::Error>> {
+    println!("cargo:rerun-if-changed=c_ext/c_ext.c");
+    println!("cargo:rerun-if-changed=c_ext/Makefile");
+
+    let (_, pg_config) =
+        pgrx_bindgen::detect_pg_config()?.pop().unwrap_or_else(|| panic!("no pg_config detected"));
+
+    let child = Command::new("make")
+        .current_dir("c_ext")
+        .env("USE_PGXS", "1")
+        .env("PROFILE", "")
+        .env("PG_CONFIG", pg_config.path().expect("pg_config must have a path"))
+        .spawn()?;
+    let output = child.wait_with_output()?;
+    if !output.status.success() {
+        eprintln!("{}", std::str::from_utf8(&output.stderr)?);
+        return Ok(ExitCode::from(output.status.code().unwrap() as u8));
+    }
+
+    let output = std::env::current_dir()?.join("c_ext");
+    println!("cargo:rustc-link-arg-cdylib={}/c_ext.o", output.display());
+
+    let out_dir = PathBuf::from(std::env::var("OUT_DIR")?);
+
+    // Force-export these symbols from the final .so
+    let vers = out_dir.join("exports.map");
+    fs::write(
+        &vers,
+        r#"
+{
+  global:
+    start_thread;
+    pg_finfo_start_thread;
+  local:
+    *;
+};
+"#,
+    )?;
+
+    let target = std::env::var("TARGET")?;
+    if target.contains("apple-darwin") {
+        println!("cargo:rustc-link-arg-cdylib=-Wl,-u,_start_thread");
+        println!("cargo:rustc-link-arg-cdylib=-Wl,-exported_symbol,_start_thread");
+        println!("cargo:rustc-link-arg-cdylib=-Wl,-exported_symbol,_pg_finfo_start_thread");
+    } else if target.contains("unknown-linux-gnu") {
+        println!("cargo:rustc-link-arg-cdylib=-Wl,--undefined=start_thread");
+        println!("cargo:rustc-link-arg-cdylib=-Wl,--undefined=pg_finfo_start_thread");
+        println!("cargo:rustc-link-arg-cdylib=-Wl,--version-script={}", vers.display());
+    }
+
+    Ok(ExitCode::SUCCESS)
+}