Introduce #[pg_extern(create_or_replace)]

[workingjubilee]

This is a followup to #554 which fixed #506, and an alternative solution to the problem raised by #584 that allows users to fix things in a more targeted manner, albeit at the requirement of a source-level change. As it is a fairly simple edit, and introduces a functionality we likely want anyways, I think this is a reasonable alternative solution.

[workingjubilee]

Per my comment over there:

I don't want to create more work for you, truly! We just are inevitably going to get "so, how about something that lets me have one function CREATEd and one function REPLACEd?" and then we'll have wanted #683 all along.

The other advantage of an in-source change is that this way, a crate builds and updates correctly even if you don't pass the right flag on the command line. It's always good to have code have its requirements like that checked in with the source.

[willmurnane]

Looks useful to have, but we should keep this CVE in mind. Maybe we can mark the functions that are created by pgx with some metadata somewhere, and make use of that to ensure we're not trying to replace non-pgx items with pgx items?

[willmurnane]

I don't like that this is a single enum, rather than a record type with a bunch of overlapping meanings. For example, IMMUTABLE, STABLE, and VOLATILE` are mutually exclusive, but there's nothing (in the Rust code) stopping one from specifying both, and getting a crash out of Postgres. Could we re-factor this to something like:

pub enum FunctionStability { Immutable, Strict, Volatile };
pub enum ParallelSafety { Safe, Unsafe, Restricted };
// ...
pub struct PgFunctionDecorations {
  create_or_replace: bool,
  stability: FunctionStability,
  leakproof: bool,
  ... // cost, no_guard, etc etc
}

Then we get more compile-time guarantees that what we generate is going to be acceptable to Postgres.

[workingjubilee]

Hm. Do you mean a crash or merely a failure to commit the transaction as Postgres rejects it?

[willmurnane]

This would presumably manifest as a failure for the extension to be installed. The work in #665 should at least make this condition easier to detect, but there's no reason we should be able to generate CREATE FUNCTION ... IMMUTABLE VOLATILE ... at all.

[workingjubilee]

Mm, I agree, but I am unenthused at the prospect of a bunch of booleans scurrying around.

[workingjubilee]

Looks useful to have, but we should keep this CVE in mind.

Correct. We used to default to CREATE OR REPLACE, and due to that CVE we changed our default to CREATE, but then issues like #584 got opened.

Maybe we can mark the functions that are created by pgx with some metadata somewhere, and make use of that to ensure we're not trying to replace non-pgx items with pgx items?

Hm. Plausible? But what about a situation where two extensions are created using PGX and one replaces a function controlled by the other?

[willmurnane]

Maybe we can mark the functions that are created by pgx with some metadata somewhere, and make use of that to ensure we're not trying to replace non-pgx items with pgx items?

Hm. Plausible? But what about a situation where two extensions are created using PGX and one replaces a function controlled by the other?

Sure, there are lots of edge cases to handle, and I'm not sure where would be best to store the data about "this function came from extension X at version Y". There's only so much we can do here - if someone creates a malicious pgx extensionfoo with version 2.0, and convinces someone to install it over the friendly foo at version 1.0, then there's only so much we can or should do to protect the user in this case. Maybe we can do better than "overwrite any function by this name" though.

[workingjubilee]

Hm.

This PR technically fixes a regression for a valid use-case, even if it's a low-security one, so I don't want to burden it with additional security requirements per se as the caution about using this is Known, and it's no longer a default, which means the 95~99% of users who only use the arguments they need will just never touch it. So I think you should open an issue for this idea so we don't forget about it and we should get some input from users who do in fact want to use create_or_replace as to what security properties they would be interested in.