Auto generate table name

[Anish9901]

Fixes #3636

Replicates the current behaviour our table add REST endpoint and generate table name if one isn't provided.

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the develop branch of the repository
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no
  visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[seancolsen]

Correct me if I'm wrong, but I think there may be a bug here. Say I begin with zero tables...

1. I create a table. It gets named Table 1.
2. I create another table. It gets named Table 2.
3. I delete the first table.
4. I create a third table. Within this function table_count will be 1 so fq_table_name will be something like public."Table 2". But public."Table 2" already exists so CREATE TABLE will fail.

Right?

[Anish9901]

You are correct @seancolsen, I've fixed the bug and added a test in 0450fbe, thanks for catching it!

[seancolsen]

Cool. I haven't reviewed thoroughly, but your changes look good from a glance. I'll leave it to Brent to review fully.

[mathemancer]

Okay, looks fine.

[seancolsen]

@Anish9901 I think I just bumped into a bug due to this change. I pulled the latest develop code and restarted my Docker container. Then I saw:

------------Setting up User Databases------------
Installing Mathesar on preexisting PostgreSQL database mathesar at host mathesar_dev_db...
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/code/mathesar/install.py", line 61, in <module>
    main()
  File "/code/mathesar/install.py", line 37, in main
    install_on_db_with_key(database_key, skip_confirm)
  File "/code/mathesar/install.py", line 47, in install_on_db_with_key
    install.install_mathesar(
  File "/code/db/install.py", line 31, in install_mathesar
    sql_install.install(user_db_engine)
  File "/code/db/sql/install.py", line 12, in install
    load_file_with_engine(engine, file_handle)
  File "/code/db/connection.py", line 62, in load_file_with_engine
    conn.execute(file_handle.read())
  File "/usr/local/lib/python3.9/site-packages/psycopg/connection.py", line 891, in execute
    raise ex.with_traceback(None)
psycopg.errors.InvalidFunctionDefinition: cannot change name of input parameter "sch_oid"
HINT:  Use DROP FUNCTION msar.add_mathesar_table(oid,text,jsonb,jsonb,text) first.

[Anish9901]

This is expected @seancolsen, You can DROP the msar, __msar schemas from the dev_db and try restarting your container.

[seancolsen]

Won't this give users the same error when they upgrade Mathesar 0.1.7 to Mathesar beta?

[Anish9901]

Yes it will, but I am expecting that the upgrade won't be easy anyway and we'd ultimately need to inform the users to drop these schemas as part of the upgrade process. At the end of our refactor if this is the only function that is breaking the upgrade we could either revert back or change the function name or we could discuss other strategies in one of our meetings.

[seancolsen]

we'd ultimately need to inform the users to drop these schemas as part of the upgrade process

This has not been my expectation. I've been solving this problem in my PRs via DROP FUNCTION IF EXISTS.

I am expecting that the upgrade won't be easy anyway

I think it's important that we prioritize making the upgrade process easy. Perhaps we need to drop msar and __msar ourselves. If that's the case, then I think we need a dedicated PR to incorporate that step into the upgrade process and into the development environment. In lieu of that change, I think we ought to use DROP FUNCTION IF EXISTS for the time being.

[seancolsen]

I opened #3650 to insert DROP FUNCTION IF EXISTS

[kgodey]

@seancolsen @Anish9901 sorry to dig up old history, but I'm cleaning up old notes and I found this in the agenda with zero notes, and nothing on tl;dv either. Was this just dropped, or was there a separate discussion about this?

[seancolsen]

@kgodey this is one of those nebulous topics that we've discussed a few times. We could discuss it more. We also could leave things as-is. I could spend a while summarizing it here, but I'm not sure it's worth the time, given that I don't have a clear understanding of what sort of info you're after and why you're asking. Basically our current practice for defining SQL functions leaves some room for things to fall through the cracks and for bugs to arise due to outdated function definitions still persisting in the database. There are a number of subtle ways this can manifest. And there are a number of potential ways we could seek to improve it.

Is there an action item here? Maybe. Probably. Is that action item tracked somewhere? No, not really. In my head sort of. Maybe in Anish's head. Maybe in Brent's head. I'd like to chat about this again, but not until Brent is back. And it's not a super high priority. So I think it's okay that nothing is tracked.

Anish and I actually discussed this 1:1 just a few days ago, as prompted by this review, and we agreed that it would be good to revisit this topic at some point once Brent returns.

I think Brent and Anish would probably share my opinion that our current setup/practice/systems/standards around function definitions and stale function signatures is not ideal. And I think all of us would agree that there's not a clear solution to this problem which easily satisfies all goals and edge cases. When we've talked about this in the past, we've had slightly differing opinions (though not very strong) on the best way to strike a balance by improving this situation. For example I proposed droping the msar schema (with cascade) at the top of 00_msar.sql. Brent felt wary of doing this and was more interested in alternate approaches.

Basically Kriti, I don't think you need to do anything here. If you want a deeper understanding of the topic, I think it would be fastest for us to chat about it 1:1 or during a team meeting.

[kgodey]

@seancolsen Sorry for not being clear enough, my interest in this is not in the topic, I'm just trying to track down the missing meeting notes (if any) i.e. what happened between this comment and this comment?

[kgodey]

@seancolsen Never mind, I found what I needed in tl;dv, it was just such a brief discussion that it didn't make the summary.