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

Temporary Tables are not listed in connection.tables in Postgres #6398

Closed
6 tasks
radeusgd opened this issue Apr 24, 2023 · 5 comments · Fixed by #6443
Closed
6 tasks

Temporary Tables are not listed in connection.tables in Postgres #6398

radeusgd opened this issue Apr 24, 2023 · 5 comments · Fixed by #6443
Assignees
Labels
--bug Type: bug -libs Libraries: New libraries to be implemented l-db-read Libraries: database reader l-db-write Libraries: database writer

Comments

@radeusgd
Copy link
Member

When a temporary table is created on a connection, it is not seen in connection.tables:

from Standard.Base import all
from Standard.Table import all
from Standard.Database import all

connect_to_postgres =
    hostname = Environment.get "ENSO_DATABASE_TEST_HOST"
    db_name = Environment.get "ENSO_DATABASE_TEST_DB_NAME"
    db_host_port = hostname.if_nothing "localhost" . split ':'
    db_host = db_host_port.at 0
    db_port = if db_host_port.length == 1 then 5432 else Integer.parse (db_host_port.at 1)
    db_user = Environment.get "ENSO_DATABASE_TEST_DB_USER"
    db_password = Environment.get "ENSO_DATABASE_TEST_DB_PASSWORD"
    case db_name.is_nothing of
        True ->
            IO.println "PostgreSQL test database is not configured. See README.md for instructions."
        False ->
            connection = Panic.rethrow <|
                IO.println "Connecting to PostgreSQL test database at "+hostname
                Database.connect (Postgres db_host db_port db_name credentials=(Credentials.Username_And_Password db_user db_password))
            connection

main =
    c = connect_to_postgres
    source = Table.new [["X", [1,2,3]]]
    non_tmp_table = c.upload_table "permament_TABLE1234" source temporary=False
    tmp_table = c.upload_table "temporary_TABLE1234" source temporary=True
    tmp_table.print

    existing = c.tables.at "Name" . to_vector
    IO.println "Permament table exists: "+(existing.contains non_tmp_table.name).to_text
    IO.println "Temporary table exists: "+(existing.contains tmp_table.name).to_text

    IO.println "Permament table query: "+(c.query non_tmp_table.name).to_text
    IO.println "Temporary table query: "+(c.query tmp_table.name).to_text

    c.execute_update 'DROP TABLE "'+non_tmp_table.name+'"'

Note this repro may need to be updated once #6326 is implemented, to use the new API.

Connecting to PostgreSQL test database at 172.28.138.202
 X
---
 1
 2
 3

Permament table exists: True
Temporary table exists: False
Permament table query: (Database Table permament_TABLE1234)
Temporary table query: (Error: (Table_Not_Found.Error 'temporary_TABLE1234' There was an SQL error: ERROR: syntax error at or near "temporary_TABLE1234"
  Pozycja: 1. [Query was: temporary_TABLE1234] True))

The table not showing up in tables is not a biggest issue (but probably ideally it should be there). The biggest issue is shown below - if we do a query - it is not recognized, because the query mechanism is using .tables to recognize if a given string is a table name or SQL query - and since it is not present there, it is wrongly treated as an SQL query text and fails.

Additionally, the method create_database_table will not be able to detect that the table with such a name already exists and report a tailored Enso error, falling back to generic SQL_Error instead.

So to improve the UX and avoid confusion, we should ensure that temporary tables are somehow accounted for. We should introduce some kind of State to the Postgres connection and append any temporary created tables to a list. Then we can have a known_tables helper which will list table names from both .tables and that State.

  • Introduce State to Postgres_Connection, holding a list of known temporary tables.
  • Fix query to rely on this state.
  • Fix any code checking for existing tables to rely on it.
  • Add tests verifying query works correctly with existing tables.
  • Add tests verifying that Table_Already_Exists is correctly reported if the clashing table is temporary.
  • (Optional) ensure that temporary tables show up in .tables
@radeusgd radeusgd added the --bug Type: bug label Apr 24, 2023
@radeusgd radeusgd self-assigned this Apr 24, 2023
@radeusgd radeusgd added -libs Libraries: New libraries to be implemented l-db-read Libraries: database reader l-db-write Libraries: database writer triage labels Apr 24, 2023
@radeusgd
Copy link
Member Author

  • (Optional) ensure that temporary tables show up in .tables

@jdunkerley do we want this? I guess it could be beneficial. I guess since we'd need to add them on-top of the results from the DB, all fields apart from Name would be NULL (well, we could have Type == "TEMPORARY" for them, I guess).

@radeusgd radeusgd moved this from ❓New to 📤 Backlog in Issues Board Apr 24, 2023
@jdunkerley jdunkerley added this to the Design Partners milestone Apr 25, 2023
@radeusgd radeusgd moved this from 📤 Backlog to 🔧 Implementation in Issues Board Apr 26, 2023
@hubertp hubertp removed the triage label Apr 26, 2023
@hubertp
Copy link
Collaborator

hubertp commented Apr 26, 2023

Removed triage since I'm guessing you agreed that this should get in.

@radeusgd
Copy link
Member Author

Removed triage since I'm guessing you agreed that this should get in.

Yep, I'm already working on this.

@enso-bot
Copy link

enso-bot bot commented Apr 27, 2023

Radosław Waśko reports a new STANDUP for yesterday (2023-04-26):

Progress: Implemented workaround ensuring that temporary tables are also included in tables listing. It should be finished by 2023-04-28.

Next Day: Next day I will be working on the #6327 task. Start work on create_database_table for Database table (DB Query -> Table transfer)

@enso-bot
Copy link

enso-bot bot commented Apr 28, 2023

Radosław Waśko reports a new STANDUP for yesterday (2023-04-27):

Progress: Wrote tests for the DB->DB create table. Applying code review suggestions. Figured out the temporary table workaround was not really needed - changed defaults on tables as agreed in discussion with James and updated the logic, removing the need for workaround. Added widget for schema in tables and tested these. It should be finished by 2023-04-28.

Next Day: Next day I will be working on the #6327 task. Implement create_database_table to pass the created tests.

@mergify mergify bot closed this as completed in #6443 Apr 29, 2023
mergify bot pushed a commit that referenced this issue Apr 29, 2023
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug -libs Libraries: New libraries to be implemented l-db-read Libraries: database reader l-db-write Libraries: database writer
Projects
Archived in project
3 participants