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.

Sign up for GitHub

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

Jump to bottom

Include type when getting argument #740

Merged
merged 5 commits into from Oct 7, 2022
Merged

Include type when getting argument #740

merged 5 commits into from Oct 7, 2022

Conversation

syvb
Copy link
Contributor

@syvb syvb commented Oct 5, 2022

This fixes #735 by passing the type of an argument when getting an argument.

@syvb syvb mentioned this pull request Oct 5, 2022
Merged
5 tasks
workingjubilee
workingjubilee previously approved these changes Oct 5, 2022
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment •
edited

Choose a reason for hiding this comment

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

This looks correct to me but I think it could use a 1-second glance from @eeeebbbbrrrr as well before it gets merged.

EDIT: My instincts about needing a second pair of eyes were correct, this is actually too slow for usage. The code needs to special-case AnyElement specifically, but I haven't the faintest idea how.

@syvb
Copy link
Contributor Author

syvb commented Oct 6, 2022 •
edited

I added an associated constant (GET_TYPOID) to FromDatum so that pg_getarg only gets the type OID for anyelement/anyarray – that should alleviate the performance problem.

@workingjubilee
Copy link
Member

Hmm. That should work, but is it possible for us to make it only happen when AnyElement's oid actually has a lookup done on it? That way we can move back to stripping the third from_datum argument in 0.6.0

@syvb
Copy link
Contributor Author

syvb commented Oct 6, 2022

I don't think it's possible to do the type lookup on demand without making FunctionCallInfo an argument to from_datum.

Another option would be to make a separate trait method for when a type OID is needed. That approach would allow for from_datum to only have 2 arguments in the future. I updated the PR to do that.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 6, 2022 •
edited

Hm. Should we use FromDatum::try_from_datum instead (.ok()ing the Result into an Option) and say that IntoDatum::is_compatible_with for AnyElement simply returns true?

@syvb
Copy link
Contributor Author

syvb commented Oct 6, 2022

try_from_datum has a trait bound that T: 'static (because it uses TypeId, which doesn't work for types with lifetimes) that pg_getarg currently doesn't have. Requiring that T: 'static for pg_getarg would mean that all #[pg_extern] functions with lifetime parameters would need to have them be 'static, which wouldn't be ideal – it results in 309 errors on my pgx extension.

@workingjubilee
Copy link
Member

...yeaaaah, that's not going to work, then.

workingjubilee
workingjubilee reviewed Oct 6, 2022
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Agreeing with my naming sense is not required, but I think makes it clearer why this function should exist.

pgx/src/fcinfo.rs Outdated Show resolved Hide resolved
pgx/src/fcinfo.rs Outdated Show resolved Hide resolved
pgx/src/datum/anyelement.rs Outdated Show resolved Hide resolved
pgx/src/datum/anyelement.rs Outdated Show resolved Hide resolved
pgx/src/datum/anyarray.rs Outdated Show resolved Hide resolved
syvb
syvb commented Oct 6, 2022
View reviewed changes
pgx/src/datum/anyarray.rs Outdated Show resolved Hide resolved
@syvb @workingjubilee
from_datum_with_typid -> from_polymorphic_datum
e58de62 
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
workingjubilee
workingjubilee approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment •
edited

Choose a reason for hiding this comment

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

We should find a cleaner solution in the future but I think this will do for right this second: it more or less brings us full circle to where we were before, but with at least a direction to go this time.

@workingjubilee workingjubilee merged commit c9a23a1 into pgcentralfoundation:develop Oct 7, 2022
@syvb syvb deleted the sv/anyelement-fix branch October 7, 2022 01:51
@eeeebbbbrrrr
Copy link
Contributor

eeeebbbbrrrr commented Oct 7, 2022 •
edited

Can we get rid of the typoid argument to FromDatum::from_datum() now?

@syvb
Copy link
Contributor Author

syvb commented Oct 7, 2022 •
edited

@eeeebbbbrrrr I think this PR fixed all the cases where typoid was needed, so it should be removable in 0.6 now.

bors bot added a commit to timescale/timescaledb-toolkit that referenced this pull request Oct 17, 2022
@bors @syvb
Merge #547
fa7b32f 
547: Upgrade to pgx 0.5.0 r=Smittyvb a=Smittyvb

Upgrades to pgx `0.5.0-beta.1` (0.5.0 will hopefully be released in 0-7 days). I originally tested on another branch with a lot of code commented out; this branch cherry picks the commits that fix errors but not the ones that comment code out.

To do:
- [x] Fix the three failing tests in `datum_utils.rs` (pgcentralfoundation/pgrx#740)
- [x] Remove workaround for lack of `Copy` on some types
- [x] Make update tester work in CI (#552)
- [x] Change pgx version from 0.5.0-beta.1 to 0.5.0
- [ ] Publish new CI docker image with pgx 0.5.0 right before merging (#571)

Future work:
- Replace `use pgx::*` with `use pgx::prelude::*`
- Use native pgx types instead of `crate::raw`
- Use `#[pg_aggregate]` instead of our own aggregate builder macro

Co-authored-by: Smitty <smitty@timescale.com>
@sumerman sumerman mentioned this pull request Feb 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

@eeeebbbbrrrr eeeebbbbrrrr Awaiting requested review from eeeebbbbrrrr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@syvb @workingjubilee @eeeebbbbrrrr