Upgrade to pgx 0.4.5 #408

rtwalker · 2022-05-03T21:58:58Z

Upgrading to the latest pgx

Closes #387

Went from 0.2.4 -> 0.3.0 -> 0.4.0 -> 0.4.3, editing the Cargo.toml, building, and testing, and the experience was largely unremarkable.

Most of the changes come from followng the "Upgrading" section of the 0.4.0 release notes.

Only thing to note is that I ran into a lot of occurrences of the following error:

 error[E0053]: method `input` has an incompatible type for trait
    --> extension/src/type_builder.rs:261:29
     |
 261 |             fn input(input: &std::ffi::CStr) -> $name<'input>
     |                             ^^^^^^^^^^^^^^^
     |                             |
     |                             expected struct `pgx::cstr_core::CStr`, found struct `std::ffi::CStr`
     |                             help: change the parameter type to match the trait: `&pgx::cstr_core::CStr`

However, just blindly swapping in the pgx::cstr_core CStr(ing)s for the std::ffi ones made Rust much happier.
Please yell at me if that's too irresponsible.

macOS support on Apple M1 Macs

I think we should be OK to close #401 as well.

pgx claims M1 support, and by virtue of being able to do

$ cargo pgx run pg14
    Stopping Postgres v14
building extension with features `pg14`
"cargo" "build" "--features" "pg14" "--no-default-features" "--message-format=json-render-diagnostics"
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s

installing extension
     Copying control file to /Users/rwalker/.pgx/14.2/pgx-install/share/postgresql/extension/timescaledb_toolkit.control
     Copying shared library to /Users/rwalker/.pgx/14.2/pgx-install/lib/postgresql/timescaledb_toolkit.so
 Discovering SQL entities
  Discovered 655 SQL entities: 9 schemas (1 unique), 521 functions, 71 types, 0 enums, 54 sqls, 0 ords, 0 hashes, 0 aggregates
     Writing SQL entities to /Users/rwalker/.pgx/14.2/pgx-install/share/postgresql/extension/timescaledb_toolkit--1.7.0-dev.sql
    Finished installing timescaledb_toolkit
    Starting Postgres v14 on port 28814
    Re-using existing database timescaledb_toolkit
psql (14.2)
Type "help" for help.

timescaledb_toolkit=#

and

$ cargo pgx test pg14
"cargo" "test" "--features" "pg14 pg_test" "--no-default-features"
    Finished test [unoptimized + debuginfo] target(s) in 0.07s
     Running unittests (/Users/rwalker/timescale/timescaledb-toolkit/target/debug/deps/timescaledb_toolkit-66d8cc098ff75a32)

running 131 tests
building extension with features `pg14 pg_test`
"cargo" "build" "--features" "pg14 pg_test" "--no-default-features" "--message-format=json-render-diagnostics"
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s

installing extension
     Copying control file to /Users/rwalker/.pgx/14.2/pgx-install/share/postgresql/extension/timescaledb_toolkit.control
     Copying shared library to /Users/rwalker/.pgx/14.2/pgx-install/lib/postgresql/timescaledb_toolkit.so
 Discovering SQL entities
  Discovered 837 SQL entities: 39 schemas (2 unique), 668 functions, 72 types, 0 enums, 58 sqls, 0 ords, 0 hashes, 0 aggregates
     Writing SQL entities to /Users/rwalker/.pgx/14.2/pgx-install/share/postgresql/extension/timescaledb_toolkit--1.7.0-dev.sql
    Finished installing timescaledb_toolkit

 ** eliding the list of 131 tests **

test result: ok. 131 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 8.87s

Stopping Postgres

where I have

$ uname -om
arm64 Darwin

I think we can claim M1 support too.

rtwalker · 2022-05-03T22:19:13Z

Hmmm a failing CI / Test Updates is a little too ironic for this PR. Not sure why it didn't occur to me to check that before pushing!

rtwalker · 2022-05-04T15:05:20Z

After digging around in our tools/update-tester crate, it looks like we define the same steps here that we list in the "Building and Installing the extension" section of the README, i.e.

quietly_run(cmd!("cargo pgx install -c {pg_config}"))

and then

quietly_run(cmd!("cargo run --manifest-path ./tools/post-install/Cargo.toml -- {pg_config}"))

But we don't have a step in there for getting the right version of pgx similar to what we have in the "Tools Setup" section.

Will try adding a let install_pgx = || { ... }; closure and dropping it in here.

JLockerman · 2022-05-04T17:43:07Z

@rtwalker I think the issue was that the installed cargo-pgx was too old, in which case it should work with the new image...

JLockerman · 2022-05-04T17:44:01Z

(we really don't want to install cargo-pgx every time we run CI, it takes much to long)

JLockerman · 2022-05-04T17:48:04Z

Oh, I see, it's trying to use the compile the old version of the extension using the current pgx, which fails. ick

rtwalker · 2022-05-04T18:03:12Z

Oh, I see, it's trying to use the compile the old version of the extension using the current pgx, which fails. ick

Yeah...

(we really don't want to install cargo-pgx every time we run CI, it takes much to long)

Agree that avoiding this would be great. Is there a way to do properly do the update-tester without having the matching version of pgx? I think for now we could get away with just installing twice (0.2.4 and 0.4.x) but I'm not sure how long that'll work for us.

418: `update-tester` requires two versions of `cargo-pgx` r=rtwalker a=rtwalker Now that #417 is merged, we can change the `update-tester` crate to require two versions of `pgx` to be installed: * a 0.2-0.3 series pgx aka "old" * a 0.4 series or newer pgx (we don't actually use this one yet, but we will after #408) Rather than insisting on having the `pgx`s installed in a specific location, the update tester takes in paths to the `*/bin/cargo-pgx` subcommands as arguments and then inspects the `Cargo.toml` during the checkout of each release to determine which one to invoke. I've also updated the `tools/build` script and the `.github/workflows/ci.yml` to use the new update tester. Went ahead and bumped the `update-tester` version to `0.2.0` so I could more easily tell the difference while testing locally, though it probably makes little practical difference. Co-authored-by: Ryan Walker <rwalker@timescale.com>

421: Don't install pgx as root or under `/` r=rtwalker a=rtwalker In #408, almost all of the CI tests are failing with permission denied errors, e.g. [here](https://github.com/timescale/timescaledb-toolkit/runs/6415824754?check_suite_focus=true#step:7:188), when trying to compile crates after running `cargo test`. This PR changes the CI Docker image to: - run all cargo commands as the "postgres" user (instead of "root") - move the pgx installations from `/pgx/0.X` to `/home/postgres/pgx/0.X` Co-authored-by: Ryan Walker <rwalker@timescale.com>

epgts · 2022-05-25T13:42:03Z

What do we have left on this? I don't understand why all the checks are failing here; I've seen all these tests (WITH 0.4!) pass on aarch64 :)

Let's merge #430 first and then use 0.4.5 here though.

Thanks!

rtwalker · 2022-05-25T13:50:44Z

What do we have left on this? I don't understand why all the checks are failing here; I've seen all these tests (WITH 0.4!) pass on aarch64 :)

Figure out how to do the dance of getting the tests for this PR to run on the CI image that is changed in this PR.

Let's merge #430 first and then use 0.4.5 here though.

Agreed!

epgts · 2022-05-25T14:23:52Z

Figure out how to do the dance of getting the tests for this PR to run on the CI image that is changed in this PR.

Ah. I remembered you were working on that but hadn't quite internalized. I don't think that's going to be doable. I think we'll need to change the image first. If that won't work either because the current code on main won't pass with the new image, can we change the image such that it'll support both ways?

I.e.

Make image support old and new style
Change main to new style
Drop old style support from image

Thanks!

Went from 0.2.4 -> 0.3.0 -> 0.4.0 -> 0.4.3, editing the Cargo.toml, building, and testing, and the experience was largely unremarkable. Most of the changes come from followng the "Upgrading" section of the [0.4.0](https://github.com/tcdi/pgx/releases/tag/v0.4.0) release notes. Only other thing to note is that I ran into a *lot* of occurrences of the following error: ``` error[E0053]: method `input` has an incompatible type for trait --> extension/src/type_builder.rs:261:29 | 261 | fn input(input: &std::ffi::CStr) -> $name<'input> | ^^^^^^^^^^^^^^^ | | | expected struct `pgx::cstr_core::CStr`, found struct `std::ffi::CStr` | help: change the parameter type to match the trait: `&pgx::cstr_core::CStr` ``` Just blindly swapping in the `pgx::cstr_core` `CStr(ing)`s for the `std::ffi` ones made Rust much happier.

Leave TODO about making this default after merge.

was needed for #408 to pass CI

rtwalker · 2022-05-26T17:15:35Z

bors r+

bors · 2022-05-26T17:33:30Z

Build succeeded:

was needed for #408 to pass CI

Now that #408 is merged, we should upgrade which pgx we use by default, i.e. which pgx is used to init the datatabases in ~/.pgx and which pgx we'll use to run our CI tests.

We added this in order for #408 to pass the CI tests. Our CI image had pgx 0.2 on its PATH, but since we were upgrading to 0.4, we wanted to use the 0.4 series pgx when running our CI tests (which use `tools/build`). This allowed us to pass our CI tests, but now that we've merged the upgrade, we don't need this, and it surely isn't useful to anyone running tests with `tools/build` locally.

Now that #408 is merged, we should upgrade which pgx we use by default, i.e. which pgx is used to init the databases in ~/.pgx and which pgx we'll use to run our CI tests.

436: change which cargo-pgx subcommand is added to PATH in CI image. r=rtwalker a=rtwalker Now that #408 is merged, we should upgrade which pgx we use by default, i.e. which pgx is used to init the databases in ~/.pgx and which pgx we'll use to run our CI tests. Co-authored-by: Ryan Walker <rwalker@timescale.com>

432: remove PATH hack in `tools/build` script r=rtwalker a=rtwalker We also needed to change `tools/build` for #408 to pass CI, but we should remove once it's merged. Co-authored-by: Ryan Walker <rwalker@timescale.com>

We added this in order for #408 to pass the CI tests. Our CI image had pgx 0.2 on its PATH, but since we were upgrading to 0.4, we wanted to use the 0.4 series pgx when running our CI tests (which use `tools/build`). This allowed us to pass our CI tests, but now that we've merged the upgrade, we don't need this, and it surely isn't useful to anyone running tests with `tools/build` locally.

432: remove PATH hack in `tools/build` script r=rtwalker a=rtwalker We also needed to change `tools/build` for #408 to pass CI, but we should remove once it's merged. Co-authored-by: Ryan Walker <rwalker@timescale.com>

rtwalker mentioned this pull request May 11, 2022

update-tester requires two versions of cargo-pgx #418

Merged

rtwalker force-pushed the rw/update-pgx branch from 251e3d9 to 276d2ff Compare May 12, 2022 19:48

rtwalker changed the title ~~Upgrade to pgx 0.4.3~~ Upgrade to pgx 0.4.4 May 12, 2022

rtwalker mentioned this pull request May 16, 2022

Don't install pgx as root or under / #421

Merged

rtwalker mentioned this pull request May 23, 2022

Build timescaledev/rust-pgx image for linux/arm64 (aarch64). #426

Closed

rtwalker and others added 4 commits May 25, 2022 13:31

Upgrade to pgx 0.4.4

3a0ff55

Upgrade to pgx 0.4.5

9ffaefc

Force pgx 0.4 tool onto PATH.

73d1a7e

Leave TODO about making this default after merge.

rtwalker force-pushed the rw/update-pgx branch from 276d2ff to 73d1a7e Compare May 25, 2022 17:53

rtwalker changed the title ~~Upgrade to pgx 0.4.4~~ Upgrade to pgx 0.4.5 May 25, 2022

epgts self-requested a review May 25, 2022 18:29

rtwalker added a commit that referenced this pull request May 25, 2022

remove PATH hack

abcaf0f

was needed for #408 to pass CI

rtwalker requested review from JLockerman and WireBaron May 25, 2022 18:32

rtwalker mentioned this pull request May 25, 2022

remove PATH hack in tools/build script #432

Merged

epgts approved these changes May 25, 2022

View reviewed changes

WireBaron approved these changes May 25, 2022

View reviewed changes

rtwalker added a commit that referenced this pull request May 26, 2022

remove PATH hack

f2a4dfe

was needed for #408 to pass CI

JLockerman approved these changes May 26, 2022

View reviewed changes

bors bot merged commit 4e6eed2 into main May 26, 2022

bors bot deleted the rw/update-pgx branch May 26, 2022 17:33

rtwalker added a commit that referenced this pull request May 26, 2022

remove PATH hack

a9bd3fb

was needed for #408 to pass CI

rtwalker mentioned this pull request May 27, 2022

change which cargo-pgx subcommand is added to PATH in CI image. #436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to pgx 0.4.5 #408

Upgrade to pgx 0.4.5 #408

rtwalker commented May 3, 2022

rtwalker commented May 3, 2022

rtwalker commented May 4, 2022

JLockerman commented May 4, 2022

JLockerman commented May 4, 2022

JLockerman commented May 4, 2022

rtwalker commented May 4, 2022

epgts commented May 25, 2022

rtwalker commented May 25, 2022

epgts commented May 25, 2022

rtwalker commented May 26, 2022

bors bot commented May 26, 2022

Upgrade to pgx 0.4.5 #408

Upgrade to pgx 0.4.5 #408

Conversation

rtwalker commented May 3, 2022

Upgrading to the latest pgx

macOS support on Apple M1 Macs

rtwalker commented May 3, 2022

rtwalker commented May 4, 2022

JLockerman commented May 4, 2022

JLockerman commented May 4, 2022

JLockerman commented May 4, 2022

rtwalker commented May 4, 2022

epgts commented May 25, 2022

rtwalker commented May 25, 2022

epgts commented May 25, 2022

rtwalker commented May 26, 2022

bors bot commented May 26, 2022