Skip to content

Conversation

@NinoFloris
Copy link
Member

Closes #6237

We were not actually bootstrapping the individual data sources, so no types for the concrete connections would get reloaded by doing multihostdatasource.ReloadTypes().

@NinoFloris
Copy link
Member Author

NinoFloris commented Oct 7, 2025

We should probably consider whether we want a direct call to connection.ReloadTypes() (as opposed to multihostdatasource.ReloadTypes()) to only reload the concrete data source.

@NinoFloris NinoFloris self-assigned this Oct 7, 2025
@vonzshik
Copy link
Contributor

vonzshik commented Oct 7, 2025

What a nice bug.

We should probably consider whether we want a direct call to connection.ReloadTypes() (as opposed to multihostdatasource.ReloadTypes()) to only reload the concrete data source.

While I highly doubt anyone will ever call connection.ReloadTypes() on read-only connection (given that what most likely happens is that you first create/update a type and when want to reload), I would probably argue that it's better for multihost scenario to wire that call to multihostdatasource.ReloadTypes() just because it's much more predictable in it's behavior (after that call you're completely sure that each new connection you get will have the new type, no matter the host it decides) plus, ideally ReloadTypes shouldn't take too long.

Now, here's another tricky question. Let's say we have two hosts and the first host is down, so we only connect to second (and it's primary). Currently, multihostdatasource.ReloadTypes() will immediately fail and throw an exception even though it's not a critical error, and the worst thing, you're unable to actually reload types for the second host. Maybe what instead we should do is:

  1. Go over each host and aggregate exceptions to throw them together afterwards
  2. In case we do fail to reload types, maybe NpgsqlDataSource.Bootstrap should set IsBootstrapped to false whenever it's called with forceReload = true to make sure the next successful Open reloads types

@NinoFloris
Copy link
Member Author

Go over each host and aggregate exceptions to throw them together afterwards

Yes this is a good point, I haven't done enough around error handling.

If we believe option 2 is better I would want to align the single host behavior to this as well.

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.

Enums not working after applying migrations with BuildMultiHost DataSource

2 participants