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

Custom types - getting "ERROR: type 'x' already exists" in multiple cases #1076

Closed
nguiard opened this issue Mar 15, 2023 · 21 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@nguiard
Copy link

nguiard commented Mar 15, 2023

Hi,
Thanks for this awesome library.

I'm fooling around with custom types.

  • This works:
use pgx::prelude::*;
use serde::{Serialize, Deserialize};

pgx::pg_module_magic!();


#[derive(Debug, Serialize, Deserialize, PostgresType)]
pub struct A;

#[pg_extern]
fn myfunc() -> A {
    A
}
  • However, this does not anymore, when you wrap the return in a Result
use pgx::prelude::*;
use pgx::spi;
use serde::{Serialize, Deserialize};

pgx::pg_module_magic!();


#[derive(Debug, Serialize, Deserialize, PostgresType)]
pub struct A;

#[pg_extern]
fn myfunc() -> Result<A, spi::Error> {
    Ok(A)
}

It compiles fine of course, but when you try to create extension, you get ERROR: type "a" already exists. (also, it's not dependent on spi, changing spi::Error to std::error::Error produces the same result).

It seems that this is tied to ordering problems in the generated sql file. Because this also fails with the same error:

use pgx::prelude::*;
use pgx::spi;
use serde::{Serialize, Deserialize};

pgx::pg_module_magic!();


#[derive(Debug, Serialize, Deserialize, PostgresType)]
pub struct B;

#[derive(Debug, Serialize, Deserialize, PostgresType)]
pub struct A;

#[pg_extern]
fn myfunc(_x: B) -> Result<A, spi::Error> {
    Ok(A)
}

But, rename struct A to struct Z, and it now works!

use pgx::prelude::*;
use pgx::spi;
use serde::{Serialize, Deserialize};

pgx::pg_module_magic!();


#[derive(Debug, Serialize, Deserialize, PostgresType)]
pub struct B;

#[derive(Debug, Serialize, Deserialize, PostgresType)]
pub struct Z;

#[pg_extern]
fn myfunc(_x: B) -> Result<Z, spi::Error> {
    Ok(Z)
}
@nguiard nguiard changed the title Custom types - getting "ERROR: type 'x' already exists" in some cases Custom types - getting "ERROR: type 'x' already exists" in multiple cases Mar 15, 2023
@eeeebbbbrrrr
Copy link
Contributor

Interesting bug. Sounds like our handling of Result<T> isn't adding itself to the dependency graph in the right place.

@eeeebbbbrrrr eeeebbbbrrrr added the bug Something isn't working label Apr 10, 2023
@eeeebbbbrrrr eeeebbbbrrrr self-assigned this Apr 10, 2023
@marcusllorente-msft
Copy link

Hi, I am encountering the same error as well. @nguiard I don't see why renaming the struct from A to Z would work, and I can't seem to make it work on my end when replicating the change from struct A to struct Z. Any insight?

@eeeebbbbrrrr
Copy link
Contributor

@marcusllorente-msft which pgrx version? Have you tried with the v0.10.0 betas?

@marcusllorente-msft
Copy link

@eeeebbbbrrrr I'm using crate v0.9.8. I tried following the steps to get the beta, but I am getting this error: pgrx-0.9.8` shouldn't be used with `cargo-pgrx-0.10.0-beta.0`, please use `pgrx = "~0.10.0-beta.0"` in your Cargo.toml`. and my Cargo.toml file includes this line: pgrx = "~0.10.0-beta.0" in the dependencies section. I am new to PGRX so apologies if this is a silly question

@eeeebbbbrrrr
Copy link
Contributor

eeeebbbbrrrr commented Jul 27, 2023

oh, yeah, you'd need to install the beta version of cargo-pgrx directly too: cargo install cargo-pgrx --version 0.10.0-beta.1 --locked

Note that I typed beta.1 here, so you'd update your Cargo.toml as well.

I don't know for certain if this is fixed in the 0.10.0 betas but there was some work around better understanding Result return types. We haven't had a chance to actually look at this issue specifically.

If beta.1 doesn't help here then I'll commit to look at this directly next week.

@marcusllorente-msft
Copy link

@eeeebbbbrrrr Just tried installing both beta.1 and beta.0, both seem to raise errors when I include the correct pgrx version:
0:pgrx-0.9.8shouldn't be used withcargo-pgrx-0.10.0-beta.1, please use pgrx = "~0.10.0-beta.1"in yourCargo.toml., perhaps I am missing something? I did the cargo install and cargo pgrx init commands too.

@eeeebbbbrrrr
Copy link
Contributor

eeeebbbbrrrr commented Jul 28, 2023

It seems like you've got cargo-pgrx installed correctly and now need to update your extension's Cargo.toml file to use that exact version. That's what the error message is trying to communicate -- maybe we need to work on the wording.

It's required that the installed cargo-pgrx and the pgrx = "x.y.z" dependency version in Cargo.toml match.

@marcusllorente-msft
Copy link

@eeeebbbbrrrr I see. I meant to say that I did include the correct dependency version in my .toml file when I tried both beta.1 and beta.0, but each one raised an error saying that pgrx-0.9.8 is the currently used dependency when I already updated that. Do you have a sample Cargo.toml file that works using this beta.1 version that I can use? Perhaps I am missing a tilde or a digit somewhere along the way

@eeeebbbbrrrr
Copy link
Contributor

Maybe you aren't picking up the proper cargo-pgrx. What does cargo pgrx --version report?

Also, all our examples work: https://github.com/pgcentralfoundation/pgrx/tree/master/pgrx-examples

@marcusllorente-msft
Copy link

@eeeebbbbrrrr it returns: cargo-pgrx 0.10.0-beta.1. My dependencies look like this:
[dependencies] pgrx = "~0.10.0-beta.1"

@eeeebbbbrrrr
Copy link
Contributor

hmm. Change that ~ to an = and see what happens.

@marcusllorente-msft
Copy link

marcusllorente-msft commented Jul 28, 2023

@eeeebbbbrrrr Just tried, getting the same error: 0: pgrx-0.9.8 shouldn't be used with cargo-pgrx-0.10.0-beta.1, please use pgrx = "~0.10.0-beta.1" in your Cargo.toml.

@eeeebbbbrrrr
Copy link
Contributor

Can I see your entire Cargo.toml file?

I wonder if maybe the pgrx-tests dependency under the [dev-dependencies] block is still pointing at 0.9.8?

@eeeebbbbrrrr
Copy link
Contributor

(we definitely need to clean up this error message)

@marcusllorente-msft
Copy link

@eeeebbbbrrrr Yes, just solved it. It was the pgrx-tests that was blocking it, had to change it to the beta.1 version. And I tried with both a ~ and an =, both work fine. Thanks! But unfortunately even with the new version, it still gives me the same error with type "x" already exists when trying to drop and create my extension. My current workaround just to manually test is to comment out the duplicate type, but as of right now I can't do the programmed unit tests :(

@eeeebbbbrrrr
Copy link
Contributor

okay. cool. We have enough information around that error message to be more specific, so I'll fix that.

And I'll look into this SQL generation ordering problem first thing next week. I appreciate your tenacity and willingness to walk through some of this prickly stuff.

@marcusllorente-msft
Copy link

@eeeebbbbrrrr thanks Eric, I appreciate the help!

eeeebbbbrrrr added a commit to eeeebbbbrrrr/pgrx that referenced this issue Jul 31, 2023
As was discussed in the comments in another issue
(pgcentralfoundation#1076 (comment)),
the error message when `cargo-pgrx`'s version doesn't match the pgrx
crate dependency version is confusing.

This cleans it up and also tells you which Cargo.toml file it is.
eeeebbbbrrrr added a commit to eeeebbbbrrrr/pgrx that referenced this issue Jul 31, 2023
…of `Result<T, _>`

When a `#[pg_extern]` function returns `Result<T, E>`, the entity graph was using the "type_id" of that whole type instead of just `T`, which is what would be in the graph.

This detects that type from the first argument of the return type and uses it in the code generation.
@eeeebbbbrrrr
Copy link
Contributor

There's a PR up for this bug over in #1241.

We'll get it reviewed and merged. It'll be included in our next release, but I am unsure when that'll be.

@marcusllorente-msft
Copy link

marcusllorente-msft commented Jul 31, 2023 via email

@thomcc
Copy link
Contributor

thomcc commented Jul 31, 2023

It's probably kind of painful.

  1. cargo install --git https://github.com/pgcentralfoundation/pgrx --branch develop --force cargo-pgrx
  2. Switch version = "0.10.0-beta.1" in your Cargo.tomls to git = "https://github.com/pgcentralfoundation/pgrx", branch = "develop".

However, this is fairly fragile, so you may want to switch --branch develop and branch = "develop" for --rev 7ab42cede38c6f0826e5419ff5322f4e5789fcbf and rev = "7ab42cede38c6f0826e5419ff5322f4e5789fcbf".

You could also checkout the latest commit on the develop branch and use a path dependency.

Note that if you mess this up, there's no way for us to detect it and give a good error message. You're probably better off waiting for the next release (although I don't know when that will be).

workingjubilee added a commit that referenced this issue Aug 1, 2023
…1240)

As was discussed in the comments in another issue:
#1076 (comment)
the error message when `cargo-pgrx`'s version doesn't match the pgrx
crate dependency version is confusing.

This cleans it up and also tells you which Cargo.toml file it is.

---------

Co-authored-by: Jubilee <[email protected]>
Co-authored-by: Thom Chiovoloni <[email protected]>
eeeebbbbrrrr added a commit that referenced this issue Aug 1, 2023
This is the third beta in the pgrx v0.10.x series. It contains a number
of soundness fixes, better error handling, more testing, and other
general code cleanup.

## Soundness Issues

* `AnyNumeric` is no longer backed by Postgres-allocated memory by
@eeeebbbbrrrr in #1216

## CI and general Testing Support

* Testing help by @eeeebbbbrrrr in
#1203
* Type testability cleanup by @eeeebbbbrrrr in
#1204
* Type roundtrip tests by @eeeebbbbrrrr in
#1185
* Stop SpiClient soundness from regressing by @workingjubilee in
#1214
* Initial valgrind support by @thomcc in
#1218
* Add a env flag that can be set to skip `#[pg_test]`-generated tests.
by @thomcc in #1239
* Ignores UI tests for MUSL environments by @BradyBonnette in
#1235
* Changes GHA workflows to use new upgraded runners by @BradyBonnette in
#1225

## General Improvements

* Add support for handling SIGINT and SIGCHLD from bgworker by @JelteF
in #1229
* Fix issue #1076: Properly handle dependency graph of `Result<T, _>` by
@eeeebbbbrrrr in #1241

## Improved Error Reporting

* Try to smartly propagate fs errors by @workingjubilee in
#1186
* Addresses cargo-pgrx error reporting by @BradyBonnette in
#1238
* Cleanup the error when cargo-pgrx version doesn't match Cargo.toml by
@eeeebbbbrrrr in #1240

## Additional Postgres Headers

* Add operator and cache related api by @VoVAllen in
#1242
* Add foreign table headers by @workingjubilee in
#1226
* Add postmaster related api by @JelteF in
#1237

## Internal Code Organization

* Modularize pgrx::spi by @workingjubilee in
#1219
* Modularize the interior of pgrx-pg-sys by @workingjubilee in
#1227

## Postgres 16-motivated Changes

* Add a workaround for the pg16/homebrew/icu4c situation by @thomcc in
#1206

## General Project Stuff

* Add security policy by @johnrballard in
#1207

## New Contributors
* @johnrballard made their first contribution in
#1207
* @VoVAllen made their first contribution in
#1242

**Full Changelog**:
v0.10.0-beta.1...v0.10.0-beta.2
@eeeebbbbrrrr
Copy link
Contributor

This has been released in v0.10.0-beta.2. https://github.com/pgcentralfoundation/pgrx/releases/tag/v0.10.0-beta.2

Enjoy!

eeeebbbbrrrr added a commit that referenced this issue Sep 5, 2023
This is the final release of v0.10.0. Thanks everyone for the beta
testing, pull requests, issues, and patience.

As always, install `cargo-pgrx` with `cargo install cargo-pgrx --locked`
and update your extension Cargo.toml files to use the `0.10.0` pgrx
dependencies.

This release includes support for Postgres 16RC1. Support for the
previous betas has been removed. As such, a fresh `cargo pgrx init` is
required.

## What's Changed Since v0.10.0-beta.4

* Fix `GetMemoryChunkContext` port by @workingjubilee in
#1273
* Better error messages when `pg_config` isn't found. by @eeeebbbbrrrr
in #1271
* Make `PostgresHash` also need `Eq` by @workingjubilee in
#1264
* Memoize git hash and extension metadata by @levkk in
#1274
* move to pg16rc1 by @eeeebbbbrrrr in
#1276
* Fix bgworker template up to 0.10.0-beta.4 by @workingjubilee in
#1270

## New Contributors
* @levkk made their first contribution in
#1274

**Changelog**:
v0.10.0-beta.4...v0.10.0

---

v0.10.0's full set of changes throughout the entire beta period are:

* Postgres 16beta1 Support by @eeeebbbbrrrr in
#1169
* Support building against macOS universal binaries by @clowder in
#1166
* list specific versions in feature gates by @eeeebbbbrrrr in
#1175
* Fix bug with converting a `pg_sys::Datum` into a `pgrx::Date` by
@eeeebbbbrrrr in #1177
* Fix Arrays with leading nulls by @eeeebbbbrrrr in
#1180
* Disable hello_versioned_so test by @workingjubilee in
#1192
* doc: fix link broken by @yihong0618 in
#1181
* fcinfo: fix incorrect length set in unsafe code by @Sasasu in
#1190
* update to pg16beta2 support by @eeeebbbbrrrr in
#1188
* Array-walking is aligned by @workingjubilee in
#1191
* Implement PGRXSharedMemory for Deque by @feikesteenbergen in
#1170
* Include security labels header by @daamien in
#1189
* Fixes macos-11 tests by @BradyBonnette in
#1197
* Pgcentralfoundation updates again by @eeeebbbbrrrr in
#1200
* Update version to 0.10.0-beta.0 by @eeeebbbbrrrr in
#1201
* Testing help by @eeeebbbbrrrr in
#1203
* Type testability cleanup by @eeeebbbbrrrr in
#1204
* Try to smartly propagate fs errors by @workingjubilee in
#1186
* Fix issue #1209 by @eeeebbbbrrrr in
#1210
* Type roundtrip tests by @eeeebbbbrrrr in
#1185
* Update version to 0.10.0-beta.1 by @eeeebbbbrrrr in
#1213
* Add a workaround for the pg16/homebrew/icu4c situation by @thomcc in
#1206
* Add security policy by @johnrballard in
#1207
* `AnyNumeric` is no longer backed by Postgres-allocated memory by
@eeeebbbbrrrr in #1216
* Modularize pgrx::spi by @workingjubilee in
#1219
* Stop SpiClient soundness from regressing by @workingjubilee in
#1214
* Add foreign table headers by @workingjubilee in
#1226
* Modularize the interior of pgrx-pg-sys by @workingjubilee in
#1227
* Initial valgrind support by @thomcc in
#1218
* Add support for handling SIGINT and SIGCHLD from bgworker by @JelteF
in #1229
* Ignores UI tests for MUSL environments by @BradyBonnette in
#1235
* Add a env flag that can be set to skip `#[pg_test]`-generated tests.
by @thomcc in #1239
* Fix issue #1076: Properly handle dependency graph of `Result<T, _>` by
@eeeebbbbrrrr in #1241
* Cleanup the error when cargo-pgrx version doesn't match Cargo.toml by
@eeeebbbbrrrr in #1240
* Add operator and cache related api by @VoVAllen in
#1242
* Addresses cargo-pgrx error reporting by @BradyBonnette in
#1238
* Update version to 0.10.0-beta.2 by @eeeebbbbrrrr in
#1244
* Bump cargo-metadata and clap-cargo by @thomcc in
#1246
* Derive Clone for Inet by @JelteF in
#1251
* Correct docs for datetime `From` impls by @workingjubilee in
#1253
* Only enable line tables for profile.dev by @thomcc in
#1249
* Remove references to master branch by @thomcc in
#1243
* Ensure bindgen gets all the `cppflags` it needs (on macOS, anyway) by
@thomcc in #1247
* update for pg16beta3 support by @eeeebbbbrrrr in
#1254
* Update version to 0.10.0-beta.3 by @eeeebbbbrrrr in
#1255
* Add proptest support by @workingjubilee in
#1258
* Misc reformatting and typo fixes by @workingjubilee in
#1260
* spi: simplify (optimize?) Datum preparation by @vrmiguel in
#1256
* Assume commutation when deriving PostgresEq by @workingjubilee in
#1261
* Demand Ord for PostgresOrd by @workingjubilee in
#1262
* Fix pgrx install causing postgresql coredump by @Sasasu in
#1263
* Update version to 0.10.0-beta.4 by @workingjubilee in
#1267

## New Contributors
* @clowder made their first contribution in
#1166
* @yihong0618 made their first contribution in
#1181
* @Sasasu made their first contribution in
#1190
* @daamien made their first contribution in
#1189
* @johnrballard made their first contribution in
#1207
* @VoVAllen made their first contribution in
#1242
* @vrmiguel made their first contribution in
#1256


**Full Changelog**:
v0.9.8...v0.10.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants