-
Notifications
You must be signed in to change notification settings - Fork 9
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
1 changed file
with
101 additions
and
0 deletions.
There are no files selected for viewing
101 changes: 101 additions & 0 deletions
101
docs/team/meeting-notes/2024/07/2024-07-11-maintainer-meeting.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| # 2024-07-11 maintainer meeting | ||
|
|
||
| **Attendees**: Brent, Pavish (partial), Anish, Sean | ||
|
|
||
| ## Current work check-in | ||
| - Anyone have any blockers or priorities that need to change? | ||
| - Pavish: I'm implementing a bunch of endpoints in the same PR, heads up to Brent that it's going to be large, but it should be simple to review, not a lot going on. | ||
| - Brent: should probably be fine. | ||
| - Pavish: can split up PR afterwards if it will be a pain to review. | ||
|
|
||
| ## Upgrading SQL in our development environments | ||
|
|
||
| I've [Sean] found myself running this command quite regularly now: | ||
|
|
||
| ``` | ||
| docker exec -i mathesar_dev_db bash -c 'psql -U mathesar' < ./db/sql/00_msar.sql | ||
| ``` | ||
|
|
||
| I need to run this after pulling the latest changes from our repo. If I don't, then sometimes things are broken. As far as I can tell, I still need to run this even if I restart Docker. Right? | ||
|
|
||
| The need to perform this step is not documented anywhere. At the very least, this is a problem for new devs. (Not a big problem right now though.) | ||
|
|
||
| But the number of SQL files is growing. We already have a few, and [Process record summaries in DB layer](https://github.com/mathesar-foundation/mathesar/pull/3561) adds `40_record_summaries.sql`. | ||
|
|
||
| I think we need a more robust way to upgrade our development environments with the latest SQL. Some kind of script or something? | ||
|
|
||
| Comment from Pavish: | ||
| - This is a bug/regression. The expected behaviour is for the migrations to happen when the service re-starts which is not working currently. I had a small discussion with Brent on this a while back. | ||
| - Here's the issue I opened today for this: https://github.com/mathesar-foundation/mathesar/issues/3674 | ||
|
|
||
| ### Conclusion | ||
| - Probably broke during Brent's work. | ||
| - We do want the SQL to be upgraded when Docker restarts. | ||
| - Need to switch to psycopg3 etc. | ||
| - Anish can work on this. | ||
| - Not a priority before beta. | ||
| - Can do a 1 line fix to temporary fix it for now. | ||
| - Anish is fixing this right now. | ||
|
|
||
| ## Brent PR review bottleneck | ||
|
|
||
| Our current process seems to be that all backend PRs get reviewed by Brent. Can Anish review some too? If so, what is our process for assigning a reviewer? | ||
|
|
||
| ### Conclusion | ||
|
|
||
| - Default to assign Brent to review backend PRs for now. | ||
| - If Brent gets too overwhelmed with PR review, then he'll delegate to Anish as needed. | ||
| - Feel free to assign Anish directly if it's an area of the codebase that you know he has worked on previously. | ||
|
|
||
|
|
||
| ## "Not found" behavior in SQL functions | ||
|
|
||
| When our SQL functions are subjected to inputs that don't yield any results, some functions return NULL, whereas others raise exceptions. We've discussed this inconsistency a few times already, and thus far we've agreed to postpone any efforts at standardizing these patterns until after beta. | ||
|
|
||
| But I'd like to present a bug which I hope will make a compelling case for raising exceptions instead of returning NULL. I think it might be worth it _now_ to use exceptions going forward. | ||
|
|
||
| Pavish [encountered a bug](https://github.com/mathesar-foundation/mathesar/pull/3648#pullrequestreview-2163203218) in a PR of mine. The bug went like this: | ||
|
|
||
| 1. The front end loaded the schemas. | ||
| 2. Due to an [unrelated bug](https://github.com/mathesar-foundation/mathesar/pull/3666), the loaded schemas had stringified OID values. That bug meant that when attempting to modify a schema, the front end would send an API request that would call `msar.patch_schema` with a stringified OID. Due to function overloading, `msar.patch_schema(sch_name text, patch jsonb)` would get invoked. | ||
| 3. Then the stringified OID would be passed through `sch_name` into `msar.get_schema_oid`. | ||
| 4. Due to the pattern of returning NULL instead of raising exceptions, `msar.get_schema_oid` would return NULL and the whole operation would basically short circuit from there. | ||
|
|
||
| The really bad part about the above bug is that the API gave no indication of failure! That's bad for users; and it left me with a tricky situation to troubleshoot, costing me time. If that function had raised an exception, I would have caught this bug in my own pre-PR QA instead of during PR review. | ||
|
|
||
| I think that erring on the side of failing early is likely to be the safer option in most cases. To be clear: I don't mean _all_ cases. There are inevitably trade-offs here. But I think that raising exceptions has sufficient merit as a pattern to justify its adoption now instead of post-beta. | ||
|
|
||
| I did a very rough audit of some of our SQL info functions to see what their behavior is: | ||
|
|
||
| | Function | "Not found" behavior | | ||
| | -- | -- | | ||
| | __msar.get_qualified_relation_name | stringified OID | | ||
| | __msar.get_qualified_relation_name_or_null | NULL | | ||
| | msar.get_attnum | NULL | | ||
| | msar.get_cast_function_name | Exception | | ||
| | msar.get_column_info | NULL | | ||
| | msar.get_column_name | NULL | | ||
| | msar.get_column_type | NULL | | ||
| | msar.get_constraint_name | NULL | | ||
| | msar.get_fresh_copy_name | Exception | | ||
| | msar.get_pk_column | NULL | | ||
| | msar.get_relation_namespace_oid | NULL | | ||
| | msar.get_relation_oid | Exception | | ||
| | msar.get_schema_name | Exception | | ||
| | msar.get_schema_oid | NULL | | ||
| | msar.get_table | NULL | | ||
| | msar.get_table_info | NULL | | ||
|
|
||
| I'm proposing that we agree on a standard of raising exceptions. I don't think this means we need to go change all the non-conformant code. But I'd like to document the standard and employ it going forward. | ||
|
|
||
| ### Discussion | ||
|
|
||
| - Brent prefers functions that deal with missing data more gracefully and don't throw exceptions. | ||
| - Sean is concerned that inconsistent exception handling could lead to confusing UI | ||
| - e.g. looking like a table rename succeeded when it didn't. | ||
| - General agreement about coding standards being useful to have, but this doesn't seem worth it right now. | ||
| - Since there's no quick agreement, let's put this discussion in the parking lot and perhaps revisit post-beta. | ||
|
|
||
| ------ | ||
|
|
||
| *At this point, Sean, Brent, and Anish switched to working through an open PR together, that discussion is not recorded in these notes.* |