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

Allow extensions to access type modifiers #684

Merged
merged 12 commits into from
Jul 2, 2024

Conversation

greg-rychlewski
Copy link
Member

@greg-rychlewski greg-rychlewski commented Jul 1, 2024

Closes #676

The main idea is:

  • Postgres returns a type modifier in the row description message along with the type oid. This modifier is where the timestamp precision information is stored. Other types can store information there as well, like decimal precision or length of varchar.
  • The decoder needs access to the type modifier so we allow the decode function to pattern match against it. I don't think it can be in the state because that is global to all decode instances.
  • I also considered having a new callback to post-process the decoding. I don't think that simplifies the code much and also arrays would have to do a second pass to apply the post-processing.
  • Super types have type modifiers as well but through my investigation the only one that has a type modifier that makes sense is array. It will be the type modifier of the elements. For other things like ranges and records, the modifier is not matching the one on the subtype. So I do not pass the modifier of the super extensions down to the sub types unless it's an array.
  • The inlined type module defines a lot of functions extension/arity and care had to be taken that adding the type modifier did not cause arities to overlap. That's why I pass nil for things like tuples where they don't care about the modifier. So that the different arity functions are preserved.

I only applied to changes for timestamp but I can do it for the other time related extensions if this seems like a good idea.

@greg-rychlewski
Copy link
Member Author

the failure on one of the jobs seems to be intermittent and happening on the main branch as well. i'll take a look to see if i can fix it but the PR itself is ok to review

lib/postgrex/protocol.ex Outdated Show resolved Hide resolved
lib/postgrex/protocol.ex Outdated Show resolved Hide resolved
@josevalim
Copy link
Member

@greg-rychlewski there is another approach which we can take here, which is to say match on mod as var!(mod) and then tell library authors that they can access the modifier as var!(mod). This means we don't need to change the pattern matching logic and that we can add other "arguments" in the future. I am don't have a preference at the moment.

Btw, does this also work if you have an array of timestamps? Can we forward the mod information down?

@greg-rychlewski
Copy link
Member Author

@greg-rychlewski there is another approach which we can take here, which is to say match on mod as var!(mod) and then tell library authors that they can access the modifier as var!(mod). This means we don't need to change the pattern matching logic and that we can add other "arguments" in the future

This sounds like a nice option to me. I will try it out in its own commit so we can take a look and revert it if it's no good.

Btw, does this also work if you have an array of timestamps? Can we forward the mod information down?

Thankfully yes. Even though we only get the modifier for the array type, Postgres makes it the same as the sub- type. I added a test for it here: https://github.com/elixir-ecto/postgrex/pull/684/files#diff-dbb00c889130c35f21ecd6d123b9dc135fc6b1478fefb50fada337914e4ef94bR1773

For other super types this doesn't seem to be the case. Ranges and Records do not have any information about the sub type modifiers.

@greg-rychlewski
Copy link
Member Author

greg-rychlewski commented Jul 1, 2024

@josevalim I gave it a shot two ways:

  1. Using the original method (and addressing your comments): please see this commit
  2. Using the var! method. Please see the latest commit. I'm not too well versed in this type of stuff so not sure if this is exactly what you had in mind. But I got the tests to pass. The one thing I notice though is I get an unused attribute warning for all the extensions not using var!(mod). I'm not sure if it's a limitation of the method or I'm just doing something wrong. this is an example of the warning:

warning: variable "mod" is unused (if the variable is not meant to be used, prefix it with an underscore)

If there is a way to clean up the var! method it does make the code changes a bit simpler. I also prefer accessing special variables rather than extending the pattern. But it's not a strong preference.

@josevalim
Copy link
Member

You can add _ = var!(mod) to each body to silence it. I will review with care tomorrow!

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

It looks great to me! The only question is if we want to use a name more specific than mod, but we can decide this once we expose it to users.

@greg-rychlewski greg-rychlewski merged commit 814140a into elixir-ecto:master Jul 2, 2024
9 checks passed
@greg-rychlewski greg-rychlewski deleted the extension_opts branch July 2, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants