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

Remove the pg_sql_graph_magic!() macro. #1591

Merged
991 changes: 497 additions & 494 deletions .github/workflows/tests.yml

Large diffs are not rendered by default.

13 changes: 6 additions & 7 deletions articles/forging-sql-from-rust.md
Original file line number Diff line number Diff line change
Expand Up @@ -338,20 +338,19 @@ for object in archive.objects() {
}
```

This list gets passed into the binary, which dynamically loads the code using something like:
This list gets passed into the binary, which builds the entity graph structure using something like:

```rust
let mut entities = Vec::default();
// We *must* use this or the extension might not link in.
let control_file = __pgrx_marker()?;
let control_file = pgrx_sql_entity_graph::ControlFile:try_from("/the/known/path/to/extname.control")?;
entities.push(SqlGraphEntity::ExtensionRoot(control_file));
unsafe {
let lib = libloading::os::unix::Library::this();
for symbol_to_call in symbols_to_call {
let symbol: libloading::os::unix::Symbol<
unsafe extern fn() -> SqlGraphEntity
> = lib.get(symbol_to_call.as_bytes()).unwrap();
let entity = symbol();
extern "Rust" {
fn $symbol_name() -> pgrx_sql_entity_graph::SqlGraphEntity;
}
let entity = unsafe { $symbol_name() };
entities.push(entity);
}
};
Expand Down
37 changes: 33 additions & 4 deletions cargo-pgrx/src/command/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,14 @@ pub(crate) fn generate_schema(
out_dot = Some(x.to_string());
};

let codegen = compute_codegen(package_manifest_path, &symbols, &lib_name, out_path, out_dot)?;
let codegen = compute_codegen(
control_file,
package_manifest_path,
&symbols,
&lib_name,
out_path,
out_dot,
)?;

let embed = {
let mut embed = tempfile::NamedTempFile::new()?;
Expand Down Expand Up @@ -214,7 +221,7 @@ pub(crate) fn generate_schema(
&package_name,
)?;

compute_sql(profile, &package_name)?;
compute_sql(profile, &package_name, &manifest)?;

Ok(())
}
Expand Down Expand Up @@ -381,6 +388,7 @@ fn first_build(
}

fn compute_codegen(
control_file_path: impl AsRef<Path>,
package_manifest_path: impl AsRef<Path>,
symbols: &[String],
lib_name: &str,
Expand All @@ -391,9 +399,20 @@ fn compute_codegen(
let lib_name_ident = Ident::new(&lib_name, Span::call_site());

let inputs = {
let control_file_path = control_file_path
.as_ref()
.to_str()
.expect(".control file filename should be valid UTF8");
let mut out = quote::quote! {
// call the marker. Primarily this ensures that rustc will actually link to the library
// during the "pgrx_embed" build initiated by `cargo-pgrx schema` generation
#lib_name_ident::__pgrx_marker();
Comment on lines +407 to +409
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should use pub extern "C" fn on this marker? rustc is more reluctant to play around with obviously-for-external-consumption interfaces. It's not something we need to fix now, however, just musing aloud.


let mut entities = Vec::new();
let control_file_entity = ::pgrx::pgrx_sql_entity_graph::SqlGraphEntity::ExtensionRoot(::#lib_name_ident::__pgrx_marker());
let control_file_path = std::path::PathBuf::from(#control_file_path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's some cursed set of cases that results in this being a malformed UTF-8 string? ...but if someone seriously asks for supporting non-UTF-8 paths in our greenfield extension build system, I'm probably going to tell them exactly where they can shove that request. Maybe we should be simplifying our code by using camino.

let control_file = ::pgrx::pgrx_sql_entity_graph::ControlFile::try_from(control_file_path).expect(".control file should properly formatted");
let control_file_entity = ::pgrx::pgrx_sql_entity_graph::SqlGraphEntity::ExtensionRoot(control_file);

entities.push(control_file_entity);
};
for name in symbols.iter() {
Expand Down Expand Up @@ -535,7 +554,11 @@ fn second_build(
Ok(())
}

fn compute_sql(profile: &CargoProfile, package_name: &str) -> eyre::Result<()> {
fn compute_sql(
profile: &CargoProfile,
package_name: &str,
manifest: &Manifest,
) -> eyre::Result<()> {
let mut bin = get_target_dir()?;
bin.push(profile.target_subdir());
bin.push(format!("pgrx_embed_{package_name}"));
Expand All @@ -545,6 +568,12 @@ fn compute_sql(profile: &CargoProfile, package_name: &str) -> eyre::Result<()> {
command.stdout(Stdio::inherit());
command.stderr(Stdio::inherit());

// pass the package version through as an environment variable
command.env(
"PGRX_PKG_VERSION",
manifest.package_version().expect("`Cargo.toml` is missing the package version property"),
);
eeeebbbbrrrr marked this conversation as resolved.
Show resolved Hide resolved

let command_str = format!("{:?}", command);
tracing::debug!(command = %command_str, "Running");
let embed_output = command
Expand Down
41 changes: 35 additions & 6 deletions pgrx-sql-entity-graph/src/control_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ to the `pgrx` framework and very subject to change between versions. While you m
use super::{SqlGraphEntity, SqlGraphIdentifier, ToSql};
use core::convert::TryFrom;
use std::collections::HashMap;
use std::path::PathBuf;
use thiserror::Error;

/// The parsed contents of a `.control` file.
Expand All @@ -43,7 +44,13 @@ pub struct ControlFile {
}

impl ControlFile {
/// Parse a `.control` file.
/// Parse a `.control` file, performing all known pgrx dynamic variable substitutions.
///
/// # Supported Dynamic Variable Substitutions
///
/// `@CARGO_VERSION@`: Replaced with the value of the environment variable `PGRX_PKG_VERSION`,
/// which is set by `cargo-pgrx` using the package version from the extension's
/// `Cargo.toml` file
///
/// ```rust
/// use pgrx_sql_entity_graph::ControlFile;
Expand All @@ -55,6 +62,14 @@ impl ControlFile {
/// ```
#[allow(clippy::should_implement_trait)]
pub fn from_str(input: &str) -> Result<Self, ControlFileError> {
fn do_var_replacements(input: &str) -> String {
input.replace(
"@CARGO_VERSION@",
&std::env::var("PGRX_PKG_VERSION")
.expect("`PGRX_PKG_VERSION` environment variable should be set"),
)
}

let mut temp = HashMap::new();
for line in input.lines() {
let parts: Vec<&str> = line.split('=').collect();
Expand All @@ -68,7 +83,7 @@ impl ControlFile {
let v = v.trim_start_matches('\'');
let v = v.trim_end_matches('\'');

temp.insert(k, v);
temp.insert(k, do_var_replacements(v));
}
let control_file = ControlFile {
comment: temp
Expand All @@ -83,13 +98,13 @@ impl ControlFile {
relocatable: temp
.get("relocatable")
.ok_or(ControlFileError::MissingField { field: "relocatable" })?
== &"true",
== "true",
eeeebbbbrrrr marked this conversation as resolved.
Show resolved Hide resolved
superuser: temp
.get("superuser")
.ok_or(ControlFileError::MissingField { field: "superuser" })?
== &"true",
== "true",
schema: temp.get("schema").map(|v| v.to_string()),
trusted: if let Some(v) = temp.get("trusted") { v == &"true" } else { false },
trusted: if let Some(v) = temp.get("trusted") { v == "true" } else { false },
};

if !control_file.superuser && control_file.trusted {
Expand All @@ -108,14 +123,28 @@ impl From<ControlFile> for SqlGraphEntity {
}

/// An error met while parsing a `.control` file.
#[derive(Debug, Clone, Error)]
#[derive(Debug, Error)]
pub enum ControlFileError {
#[error("Filesystem error reading control file")]
IOError {
#[from]
error: std::io::Error,
},
#[error("Missing field in control file! Please add `{field}`.")]
MissingField { field: &'static str },
#[error("Redundant field in control file! Please remove `{field}`.")]
RedundantField { field: &'static str },
}

impl TryFrom<PathBuf> for ControlFile {
type Error = ControlFileError;

fn try_from(value: PathBuf) -> Result<Self, Self::Error> {
let contents = std::fs::read_to_string(value)?;
ControlFile::try_from(contents.as_str())
}
}

impl TryFrom<&str> for ControlFile {
type Error = ControlFileError;

Expand Down
2 changes: 1 addition & 1 deletion pgrx-tests/src/framework.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ fn install_extension() -> eyre::Result<()> {
.arg("--pg-config")
.arg(pg_config.path().ok_or(eyre!("No pg_config found"))?)
.stdout(Stdio::inherit())
.stderr(Stdio::piped())
.stderr(Stdio::inherit())
eeeebbbbrrrr marked this conversation as resolved.
Show resolved Hide resolved
.env("CARGO_TARGET_DIR", get_target_dir()?);

if let Ok(manifest_path) = std::env::var("PGRX_MANIFEST_PATH") {
Expand Down
2 changes: 1 addition & 1 deletion pgrx-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub use framework::*;
pub mod proptest;

#[cfg(any(test, feature = "pg_test"))]
pgrx::pg_sql_graph_magic!();
pgrx::pg_module_magic!();

#[cfg(test)]
pub mod pg_test {
Expand Down
2 changes: 0 additions & 2 deletions pgrx-tests/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,3 @@ mod variadic_tests;
mod xact_callback_tests;
mod xid64_tests;
mod zero_datum_edge_cases;

pgrx::pg_magic_func!();
44 changes: 9 additions & 35 deletions pgrx/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,19 @@ const _: () = {
///
/// </pre></div>
///
/// This calls both [`pg_magic_func!()`](pg_magic_func) and [`pg_sql_graph_magic!()`](pg_sql_graph_magic).
/// This calls [`pg_magic_func!()`](pg_magic_func).
#[macro_export]
macro_rules! pg_module_magic {
() => {
$crate::pg_magic_func!();
$crate::pg_sql_graph_magic!();

// A marker function which must exist in the root of the extension for proper linking by the
// "pgrx_embed" binary during `cargo-pgrx schema` generation.
#[inline(never)] /* we don't want DCE to remove this as it *could* cause the compiler to decide to not link to us */
#[doc(hidden)]
pub fn __pgrx_marker() {
eeeebbbbrrrr marked this conversation as resolved.
Show resolved Hide resolved
// noop
}
};
}

Expand Down Expand Up @@ -259,39 +266,6 @@ macro_rules! pg_magic_func {
};
}

/// Create necessary extension-local internal marker for use with SQL generation.
///
/// <div class="example-wrap" style="display:inline-block">
/// <pre class="ignore" style="white-space:normal;font:inherit;">
///
/// **Note**: Generally [`pg_module_magic`] is preferred, and results in this macro being called.
/// This macro should only be directly called in advanced use cases.
///
/// </pre></div>
#[macro_export]
macro_rules! pg_sql_graph_magic {
() => {
// A marker which must exist in the root of the extension.
#[doc(hidden)]
pub fn __pgrx_marker() -> $crate::pgrx_sql_entity_graph::ControlFile {
use ::core::convert::TryFrom;
let package_version = env!("CARGO_PKG_VERSION");
let context = include_str!(concat!(
env!("CARGO_MANIFEST_DIR"),
"/",
env!("CARGO_CRATE_NAME"),
".control"
))
.replace("@CARGO_VERSION@", package_version);

let control_file =
$crate::pgrx_sql_entity_graph::ControlFile::try_from(context.as_str())
.expect("Could not parse control file, is it valid?");
control_file
}
};
}
Comment on lines -271 to -293
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent.


pub(crate) static UTF8DATABASE: Lazy<Utf8Compat> = Lazy::new(|| {
let encoding_int = unsafe { pgrx_pg_sys::GetDatabaseEncoding() };
match encoding_int as core::ffi::c_uint {
Expand Down
Loading