Enhancement to prevent cargo from recompiling allowlisted dependencies.

[anth0nyleung]

Issue

We are facing the following problem today when using allowlisted dependencies.

We understand that all the dependency crates of pgrx (and those of the allowlisted crates) need to be compiled on cold start and that's a fixed cost. Suppose we have a fixed list of allowlisted crates, we want to prevent cargo from re-compiling any dependencies on consecutive build of plrust functions.

The way PL/Rust works today is that it adds crates into the Cargo.toml [dependencies] section based on what is specified in the [dependencies] section of the function definition. I believe we could suffer from the feature unification problem in certain cases.

For example, given crateA , crateB as allowlisted crates, and assume they both depends on crateC but crateA requires featureX in crateC and crateB requires featureY in crateC.

If we first create a function with just using crateA, the Rust fingerprint of crateC would say featureX is enabled. However, if we then create a function with just crateB, then the existing rlib for crateC cannot be re-used because now crateC needs to have featureY enabled which is not the case here. Similarly, if it is followed by creating a function which uses both crateA and crateB, the existing 2 version of rlib for crateC would not be compatible, and again needs to be recompiled because we need a crateC to to have both featureX and featureY enabled.

Proposal

The proposal i have is to use the same dependency set in Cargo.toml for every new function creates when allowlisted dependencies is a non-empty set.

E.g. Suppose the allowlist looks like this

CrateA = { version = "1.0", features = [ "featureX", "featureY" ] }
CrateB = "2.0"

When user request to create a function which uses dependencies with the above allowlist, plrust still checks if the requested dependencies are allowed. But when plrust creates the crate for it, we always use the same value for "dependencies" in Cargo.toml

CrateA = { version = "1.0", features = [ "featureX", "featureY" ] }
CrateB = "2.0"

I believe this is also how Rust playground manage to avoid rebuilding any dependencies with its top 100 crates that are supported by it.

[workingjubilee]

@anth0nyleung This is an interesting problem!

The slight problem is that sometimes a crate has multiple features, like... oh... notorious crate pgrx... that has "non-additive features". The reason the resolver has moved to a slightly different logic of how it splits up crates and does multiple compilations of them is partly that it was simply too eager in cases where unification is never useful, but also that if they are incorrectly unified, two sets of crate features can cause this sort of thing to be reached:

https://github.com/rhaiscript/rhai/blob/6d6390dee1e7d8f66ce27340246a0e02905795e1/src/lib.rs#L461-L463

Otherwise it is fairly eager to unify crate features, but this can cause other problems even if we don't hit compile errors, as enabling features can subtly change the behavior of a crate!

I suppose we could have this as a configurable option? There are some allowlists for which following this proposal will Basically Always Be Okay. There are others which it would not be. We'd have to rely on the admin being thoughtful about that.

[anth0nyleung]

That's interesting.

The biggest problem im trying to avoid is re-building pgrx-pg-sys which is a very costly operation. It's very undesirable when this kind of unification problem happen because of say we support the serde crate that yields a different set of unified feature and at the end result in re-building pgrx-pg-sys. To make it worse, it takes quite some effort and time to find out what is causing the rebuild if it happens. I feel like there is a general lack of useful tools to debug this kind of issue.

I think making it configurable makes sense since this falls into the bucket of a nice-to-have feature. We will also make it clear in the documentation that this does not guarantee to completely avoid such problem but in most cases it should work like you said. I can go ahead to start working on this if it sounds good.

[workingjubilee]

Hm. If the primary issue is pgrx-pg-sys, one of the things we can do is pay close attention to the features used by pgrx-pg-sys and its dependencies a bit more, so that it is more cagey about unifying them. I would be happy to review this feature but I would prefer a first-pass on the cargo tree there.

$ cargo tree -p pgrx-pg-sys
pgrx-pg-sys v0.12.0-alpha.1 (~/pgrx/pgrx-pg-sys)
├── libc v0.2.152
├── memoffset v0.9.0
│   [build-dependencies]
│   └── autocfg v1.1.0
├── pgrx-macros v0.12.0-alpha.1 (proc-macro) (~/pgrx/pgrx-macros)
│   ├── pgrx-sql-entity-graph v0.12.0-alpha.1 (~/pgrx/pgrx-sql-entity-graph)
│   │   ├── convert_case v0.6.0
│   │   │   └── unicode-segmentation v1.10.1
│   │   ├── eyre v0.6.11
│   │   │   ├── indenter v0.3.3
│   │   │   └── once_cell v1.19.0
│   │   ├── petgraph v0.6.4
│   │   │   ├── fixedbitset v0.4.2
│   │   │   └── indexmap v2.1.0
│   │   │       ├── equivalent v1.0.1
│   │   │       └── hashbrown v0.14.3
│   │   ├── proc-macro2 v1.0.78
│   │   │   └── unicode-ident v1.0.12
│   │   ├── quote v1.0.35
│   │   │   └── proc-macro2 v1.0.78 (*)
│   │   ├── syn v2.0.52
│   │   │   ├── proc-macro2 v1.0.78 (*)
│   │   │   ├── quote v1.0.35 (*)
│   │   │   └── unicode-ident v1.0.12
│   │   ├── thiserror v1.0.56
│   │   │   └── thiserror-impl v1.0.56 (proc-macro)
│   │   │       ├── proc-macro2 v1.0.78 (*)
│   │   │       ├── quote v1.0.35 (*)
│   │   │       └── syn v2.0.52 (*)
│   │   └── unescape v0.1.0
│   ├── proc-macro2 v1.0.78 (*)
│   ├── quote v1.0.35 (*)
│   └── syn v2.0.52 (*)
├── pgrx-sql-entity-graph v0.12.0-alpha.1 (~/pgrx/pgrx-sql-entity-graph) (*)
├── serde v1.0.197
│   └── serde_derive v1.0.197 (proc-macro)
│       ├── proc-macro2 v1.0.78 (*)
│       ├── quote v1.0.35 (*)
│       └── syn v2.0.52 (*)
└── sptr v0.3.2
[build-dependencies]
├── bindgen v0.69.4
│   ├── bitflags v2.4.2
│   ├── cexpr v0.6.0
│   │   └── nom v7.1.3
│   │       ├── memchr v2.7.1
│   │       └── minimal-lexical v0.2.1
│   ├── clang-sys v1.7.0
│   │   ├── glob v0.3.1
│   │   ├── libc v0.2.152
│   │   └── libloading v0.8.1
│   │       └── cfg-if v1.0.0
│   │   [build-dependencies]
│   │   └── glob v0.3.1
│   ├── itertools v0.12.1
│   │   └── either v1.9.0
│   ├── lazy_static v1.4.0
│   ├── lazycell v1.3.0
│   ├── proc-macro2 v1.0.78 (*)
│   ├── quote v1.0.35 (*)
│   ├── regex v1.10.3
│   │   ├── regex-automata v0.4.5
│   │   │   └── regex-syntax v0.8.2
│   │   └── regex-syntax v0.8.2
│   ├── rustc-hash v1.1.0
│   ├── shlex v1.3.0
│   └── syn v2.0.52 (*)
├── clang-sys v1.7.0 (*)
├── eyre v0.6.11 (*)
├── pgrx-pg-config v0.12.0-alpha.1 (~/pgrx/pgrx-pg-config)
│   ├── cargo_toml v0.19.1
│   │   ├── serde v1.0.197 (*)
│   │   └── toml v0.8.10
│   │       ├── serde v1.0.197 (*)
│   │       ├── serde_spanned v0.6.5
│   │       │   └── serde v1.0.197 (*)
│   │       ├── toml_datetime v0.6.5
│   │       │   └── serde v1.0.197 (*)
│   │       └── toml_edit v0.22.6
│   │           ├── indexmap v2.1.0 (*)
│   │           ├── serde v1.0.197 (*)
│   │           ├── serde_spanned v0.6.5 (*)
│   │           ├── toml_datetime v0.6.5 (*)
│   │           └── winnow v0.6.5
│   ├── eyre v0.6.11 (*)
│   ├── home v0.5.9
│   ├── owo-colors v3.5.0
│   ├── pathsearch v0.2.0
│   │   ├── anyhow v1.0.79
│   │   └── libc v0.2.152
│   ├── serde v1.0.197 (*)
│   ├── serde_derive v1.0.197 (proc-macro) (*)
│   ├── serde_json v1.0.112
│   │   ├── itoa v1.0.10
│   │   ├── ryu v1.0.16
│   │   └── serde v1.0.197 (*)
│   ├── thiserror v1.0.56 (*)
│   ├── toml v0.8.10 (*)
│   └── url v2.5.0
│       ├── form_urlencoded v1.2.1
│       │   └── percent-encoding v2.3.1
│       ├── idna v0.5.0
│       │   ├── unicode-bidi v0.3.15
│       │   └── unicode-normalization v0.1.22
│       │       └── tinyvec v1.6.0
│       │           └── tinyvec_macros v0.1.1
│       └── percent-encoding v2.3.1
├── proc-macro2 v1.0.78 (*)
├── quote v1.0.35 (*)
├── shlex v1.3.0
├── syn v2.0.52 (*)
└── walkdir v2.4.0
    └── same-file v1.0.6

[workingjubilee]

@anth0nyleung Can you relate any examples on what features appearing in dependency sets are triggering pgrx-pg-sys rebuilds, specifically?

[anth0nyleung]

@workingjubilee - sorry it took me a while to get back to you

I think our goal is to avoid any dependencies rebuilds, not specifically to pgrx-pg-sys, but we would really like pgrx-pg-sys to not ever rebuild (unless we upgrade it to a new version)

One example i can give is the rand crate.

When rand crate is used in a function. The crate getrandom enables the std feature.

(24-03-20 2:23:59) <0> [~/plrust-crate]
~/plrust-crate $ cat with-rand | grep getrandom
│           │   │   │   └── getrandom feature "default"
│           │   │   │       └── getrandom v0.2.12
    │   │           └── getrandom feature "default" (*)
    │   ├── rand feature "getrandom"
    │   │   └── rand_core feature "getrandom"
    │       ├── getrandom feature "std"
    │       │   └── getrandom v0.2.12 (*)
    │       └── rand_core feature "getrandom" (*)

However, plrust-trusted-pgrx also depends on getrandom, but the getrandom crate in this case only enables the default feature, which does not include std.

(24-03-20 2:26:03) <0> [~/plrust-crate]
~/plrust-crate $ cat only-plrust-trusted-pgrx | grep getrandom
            │   │   │   └── getrandom feature "default"
            │   │   │       └── getrandom v0.2.12

In this case, suppose i have a function that was created without using rand. Now the fingerprint of getrandom does not have std. But when rand is used as dependencies in a new function, it'll case a rebuild on these dependencies

...
Compiling getrandom v0.2.12
Compiling uuid v1.8.0
Compiling pgrx v0.11.0
Compiling plrust-trusted-pgrx v1.2.7

When this happens on a host with limited CPU, this can make a huge differences. We observed that a function can take close to 3 mins to compile when this rebuild happens, as opposed to just 5 seconds when there is no rebuild of crates.

[workingjubilee]

Hmm. I wonder how pgcentralfoundation/pgrx#1616 will affect the build times of PL/Rust functions.

[anth0nyleung]

That looks like a pretty nice improvement. Looks like plrust 1.2.8 is still on pgrx 0.11.0. Can it work with 0.11.3 or develop branch of pgrx? I can try testing it. In the past my experience of compiling a PL/Rust function in the worst case is 45 hours... This is caused by pgrx-pg-sys, wondering how much improvement that change can bring