-
Notifications
You must be signed in to change notification settings - Fork 817
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
Simplify type autoloading with pgxpool #2048
base: master
Are you sure you want to change the base?
Conversation
8714ac7
to
c98f597
Compare
A recommended way of using this would be something like:
Which, compared to previously, would:
Together this should result in a significant improvement both to startup times and whenever new connections are being added to the connection pool, whenever custom types are being registered. |
I think I'd like to wait and see what happens with #2046 before looking too much into this. But there are a few things that come to mind.
|
Agreed, let's get the other PR sorted before finessing this PR. Related array types should be handled by the new loader. I haven't experimented with this in depth, as I haven't run into any problems with the code, but if there is an example/test where array types aren't supported, I can remedy that, probably in the other PR. |
Right, I expect it would work - once the underlying type is loaded. But I wasn't sure if the |
If it works correctly, this line should collect any dependent array types. I can look at adding a test in the other PR to explicitly ensure this works. |
What I mean is, what happens when it tries to load |
90826ee
to
a57930b
Compare
I see what you mean. If |
a57930b
to
241ed08
Compare
Thinking a bit more about manual registration of certain types, and how to reduce the number of queries there: what do you think of adding something like a func (c *Config) WithManualTypeRegistration(typeName string, f func(c conn.Conn, oid uint32) error) *Config {
...
} (or perhaps passing in the typemap object instead of the connection, if you prefer) |
241ed08
to
f5bcb7b
Compare
I have rebased this on top of the other PR, and added a comment relating to the autoload config settings to clarify that custom type registration done in |
f5bcb7b
to
2538c40
Compare
b8b9930
to
59927e7
Compare
Thinking about https://pkg.go.dev/sync#OnceValue : I can see how this can replace the use of a mutex. Were you thinking that the pgxpool's struct would contain a function used to register the types? And that if the reuse flag was set, that function would be replaced by the The primary question is whether the user provides the autoload information via the pgxpool's config, or via some other mechanism. |
Yes, something like this. Though since pgxpool already has a AfterConnect hook it might just be using AfterConnect in a specific way.
The reuse flag wouldn't have to exist. The function would use OnceValue internally. From pgxpool's point of view, it has a function it calls to get (or get and register) the type information. This technique could be documented, or maybe even a new standalone function that encapsulates this concept and returns a type loading function that internally uses OnceValue. Potentially, this lets the user control autoload and type reuse without pgxpool needing any explicit support. |
Without reuse being optional, how would heterogeneous servers be supported, where oids could differ? Whether it's via a config setting or environment variable, if this is a possible situation, we should support it, shouldn't we? If not, I certainly agree that it would be nice to eliminate the need to support such a workflow. This was based on what you suggested earlier. |
Reuse would still be optional. It would be up to the caller to implement. Here is what I'm thinking: makeLoadTypesOnce := func() func(ctx context.Context, conn *pgx.Conn) error {
var mux sync.Mutex
var loaded bool
var types []*pgtype.Type
return func(ctx context.Context, conn *pgx.Conn) error {
mux.Lock()
defer mux.Unlock()
if loaded {
return types, nil
}
var err error
types, err = conn.LoadTypes(ctx, "mytype", "myothertype")
if err != nil {
return err
}
loaded = true
return nil
}
}
dbconfig.AfterConnect = makeLoadTypesOnce() I haven't actually tested it, but I think it should work. I wasn't able to figure out a way to get Also, prototyping this code made me even more in favor of this logic being activated through the existing dbconfig.AfterConnect = func(ctx context.Context, conn *pgx.Conn) error {
pgxuuid.Register(conn.TypeMap())
return nil
} It's using It could be modified as follows to load types once after registering the custom UUID type: loadTypesOnce := makeLoadTypesOnce()
dbconfig.AfterConnect = func(ctx context.Context, conn *pgx.Conn) error {
pgxuuid.Register(conn.TypeMap())
err := loadTypesOnce(ctx, conn)
if err != nil {
return err
}
return nil
} Not sure if we would want a function that creates / simplifies the load once logic or not. It would be more convenient. But I can imagine different applications having slightly different preferences so it might be easier to document the pattern instead. |
We can make this something where each user needs to write/adapt their AfterConnect to do this. I just thought that type registration was such a common pattern that we could ensure it would done in a consistent way through providing helpers to manage the autoloading, and to keep AfterConnect free for anything special they want done apart from type registration. |
59927e7
to
d3dbf83
Compare
@jackc I have made a helper which can be used to build an I think this strikes a reasonable balance, where this helper can keep things sufficiently DRY while still allowing people to decide whether to chain this with other I'm also interested in what you think of the environment variable. I do think that since we aren't using the pgxpool config for this any more, it wouldn't be possible for a DSN to control the reuse of type information, so an optional environment variable seems like a good way to let this setting be controlled by end users. |
d3dbf83
to
bcee690
Compare
f366731
to
e5ee693
Compare
If the approach shown here is one you are happy with, it might actually make sense for me to integrate this with #2049 , as the helper function's signature will change when support for custom registration functions is added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not in favor of the environment variable for two reasons.
- The caller could read an environment variable if they wanted to. This could be as simple as
AutoloadAfterConnect([]string{"mytype", "myothertype"}, os.Getenv("PGXPOOL_REUSE_TYPEMAP") == "y")
. I don't think we gain anything by having it internal to this function. - pgx's use of environment variables is limited to when creating / parsing the connection / pool config. An environment variable is a global variable, and isolating their usage to a single point makes the code easier to reason about.
If the approach shown here is one you are happy with, it might actually make sense for me to integrate this with #2049 , as the helper function's signature will change when support for custom registration functions is added.
I do think we will want to wait to merge until we also have figured out how to handle custom registration for types like hstore
and PostGIS types.
I think the custom registration functions are worth looking into.
I also think it's worth considering letting LoadTypes
load base types and have it leave the Codec
empty. The caller could populate the Codec
for base types. This would allow a single query to suffice for all type registration.
It also might be worth considering doing nothing. There are relatively few base types that will be used by any given project relative to all the derived types. A few extra queries might be inconsequential relative to the complexity of optimizing them away.
The reason for wanting the environment variable is because I expect there are a significant number of projects which use |
e5ee693
to
089aee3
Compare
I'd prefer to hold off on the environment variable. We can always add it later, but once it's there we can't remove it. |
089aee3
to
e6e5981
Compare
OK, I've removed the environment variable reference. |
There is still the race condition mentioned above. But there is one other thing that needs to be figured out that was brought to my attention by #2089. |
I'll take a look at that race condition. I didn't notice your comment, sorry. Relating to state mutation: if types and codecs shouldn't be mutated, should they have a |
While LoadTypes is powerful on its own, retriving all salient type information in a single query, it is particularly powerful and useful when users of pgxpool can use this. To streamline its use, provide a helper function in pgxpool suitable for plugging directly into the AfterConnect configuration settings. It will load the types and register them, also reusing them if appropriate.
e6e5981
to
5124091
Compare
Seems reasonable.
I would make it an optional additional interface a |
Provide some backwards-compatible configuration options for pgxpool
which streamlines the use of the bulk loading of custom types:
load for each connection, automatically also loading any other
types these depend on.
avoiding the need to perform any further queries as new connections
are created.
ReuseTypeMaps is disabled by default as in some situations, a
connection string might resolve to a pool of servers which do not share
the same type name -> OID mapping.