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

Add client for SQL #511

Merged
merged 5 commits into from
Jul 5, 2023
Merged

Add client for SQL #511

merged 5 commits into from
Jul 5, 2023

Conversation

russtoku
Copy link
Contributor

A first attempt at a client for interacting with an SQL REPL to satisfy issue #432.

Although built using PostgresQL's psql command line utility, it should work with MySQL's mysql, Oracle's oci, and others. But I did not try any other.

@russtoku
Copy link
Contributor Author

I'm still unsure about how the next-in-queue function plays into the REPL interaction. I copied remote/stdio.fnl to remote/stdio-rt.fnl to get the responses from the psql REPL displayed in the Conjure log buffer. I couldn't figure out why I don't see the psql prompt in the responses from the on-stdout or on-stderr callbacks.

So, this is more of a proof-of-concept thing.

@russtoku
Copy link
Contributor Author

Doh! psql doesn't send a prompt when not in an interactive terminal.

@russtoku
Copy link
Contributor Author

russtoku commented Jul 4, 2023

Added a SQL statement with meta-command for psql to demo that <localleader>ei (interrupt REPL) stops the meta-command and doesn't kill the REPL. Updated conjure-client-sql-stdio doc to reflect this.

Also, noticed that when stopping the REPL, the REPL process became a zombie (half-dead -- you can find it with ps but it's non-functional). It looks like just calling process_kiill without handle:close will clean up resourses automatically and lets the REPL process die normally.

@Olical
Copy link
Owner

Olical commented Jul 5, 2023

This looks wonderful! You caught everything I think, all the right docs, config and files. Amazing!

And great catch on the zombie process, that'd be easy to miss. The next-in-queue system I think was a misguided attempt at making evals more robust and separated from user interactions, so things could be queued up by the user or the system and then executed as quickly as possible by some other sub-system.

I think it didn't really play out how I wanted so now it's tech debt. I'm okay with the stdio-rt copy for now but only really because I want to completely rewrite stdio which would hopefully supersede stdio+stdio-rt, supporting everything we need from both in a nicer way. Again I refer to #500 and something I need to do eventually 🥲

For now I'm focussing on some Fennel tooling improvements to make the development nicer, but I'll get to that eventually.

I'm going to merge this to develop to get it in people's hands, once again, amazing work and thank you!

@Olical Olical merged commit ea8afd5 into Olical:develop Jul 5, 2023
@russtoku russtoku deleted the dev-sql branch July 5, 2023 17:17
@tssm
Copy link

tssm commented Jul 13, 2023

Hey @russtoku, I just wanna thank you for this! I can confirm it works with MySQL ❤️

@russtoku
Copy link
Contributor Author

@tssm , that's good news! Glad it works for you!

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.

3 participants