Skip to content

Better reporting for database connection issues #344

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

Closed
mmoncure opened this issue Apr 15, 2025 · 5 comments · Fixed by #348
Closed

Better reporting for database connection issues #344

mmoncure opened this issue Apr 15, 2025 · 5 comments · Fixed by #348
Labels
bug Something isn't working

Comments

@mmoncure
Copy link

Bug report

test.sql:
INSERT INTO foo DEFAULT VALUES ON CONFLICT DO NOTHING;

...is parsing, but takes ~ 10 seconds to parse. This is slow enough to be considered bug IMNSHO.

@mmoncure mmoncure added the bug Something isn't working label Apr 15, 2025
@psteinroe
Copy link
Collaborator

Thanks for reporting! Can you share a bit about your setup / config / logs? This is definitely not normal.

@mmoncure
Copy link
Author

Sure:
O/S mac

version: 0.3.1 (via pgtools binary suite, the compiling instructions have been pulled, heh)
lrwxr-xr-x@ 1 merlin.moncure staff 34B Apr 9 15:27 pgtools -> postgrestools_aarch64-apple-darwin

time ~/pgtools/pgtools check ~/Downloads/test.sql
Checked 1 file in 10s. No fixes applied.

real 0m10.077s
user 0m0.011s
sys 0m0.030s

command:
DT7FN214T3:output merlin.moncure$ ~/pgtools/pgtools -V
Version: 0.3.1

Checking, postgrestools.jsonc
I see: "connTimeoutSecs": 10

ok, so there we go. It's not super clear that INSERT introduced a connection, so maybe we could look at this as feedback issue -- if connection is attempted, and especially if fails, we ought to have feedbac.

restoring the connection, I get:

✖ relation "foo" does not exist

2 │ INSERT INTO foo DEFAULT VALUES ON CONFLICT DO NOTHING;
│ ^^^
3 │

✖ Error Code: 42P01

Ok, this makes sense, but it raises more questions; why does simple syntax checking write to the database? This is source level initialization script.

@psteinroe
Copy link
Collaborator

That makes sense! Can you try to pass --skip-db (ref)? This will skip all checks that require a database connection.

@mmoncure
Copy link
Author

mmoncure commented Apr 15, 2025

Yep, that definitely addresses the symptoms. I guess I would register two points of concern:

#1: The default behavior of 'check', to execute DML is dangerous IMO. Particularly if psql directives are not honored.. the structure of this file was something like:
\if init=1
create this...
insert that...
\endif

I can understand referencing a database for things like column completion, but not in this context. This seems a bit dangerous IMO, (based on very superficial analysis); need to study this more. My gut says parser/checker use the database as a parse cache of various objects rather try and execute DML unless you very specifically tell it to do that.

#2: As noted upthread, connection failure fails w/o output. From command line perspective, it ought to just fail? It might be perceived (as I did) as slow performance

@psteinroe
Copy link
Collaborator

thanks for your feedback!

The default behavior of 'check', to execute DML is dangerous IMO.

we do not execute anything. We are just PREPAREing the statements. This is minimal impact on the database, and all prepared statements are dropped when the sessions ends. This is a common practice of dev tools like sqlx.

As noted upthread, connection failure fails w/o output. From command line perspective, it ought to just fail? It might be perceived (as I did) as slow performance

Fully agree here! We need to report that better. Will keep this issue open for that.

@psteinroe psteinroe changed the title Extremely slow performance parsing, INSERT...ON CONFLICT Better reporting for database connection issues Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants