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

Can execute multiple statements in a single query #601

Closed
alxndrsn opened this issue May 10, 2024 · 8 comments
Closed

Can execute multiple statements in a single query #601

alxndrsn opened this issue May 10, 2024 · 8 comments

Comments

@alxndrsn
Copy link
Contributor

Expected Behavior

Attempting to run multiple statements should throw an Error, and the statement should not be executed.

Current Behavior

Attempting to run multiple statements should throw an Error, and the statements are executed.

Possible Solution

I think a fix would require a change in node-postgres to force use of extended queries when there is no prepared statement or query parameters.

Steps to Reproduce

  test('allows multiple statements in a single query, but throws error', async (t) => {
    const pool = await createPool(t.context.dsn, {
      driverFactory,
    });

    const result1 = await pool.anyFirst(sql.unsafe`
      SELECT name FROM person
    `);

    t.deepEqual(result1, []);

    const error = await t.throwsAsync(
      pool.query(sql.unsafe`
        INSERT INTO person (name) VALUES('alice');
        INSERT INTO person (name) VALUES('bob');
      `),
    );

    t.true(error instanceof InvalidInputError);

    const result = await pool.anyFirst(sql.unsafe`
      SELECT name FROM person
    `);

    t.deepEqual(result, ['alice', 'bob']);
  });

Logs

Passing test can be seen at:

@alxndrsn
Copy link
Contributor Author

@gajus
Copy link
Owner

gajus commented May 13, 2024

Unfortunately, the only way to support this at Slonik-level would be if we had a SQL tokenizer.

I long wanted SQL tokenizer for other use cases, but there aren't any that have good enough coverage for production adoption.

@gajus
Copy link
Owner

gajus commented May 14, 2024

@alxndrsn another option to prevent this at Slonik level (assuming brianc/node-postgres#3214 does not get merged), would be to auto generate statement name based on the query. The overhead would be pretty minimal and it would achieve more or less the same effect as the proposed pg patch.

@alxndrsn
Copy link
Contributor Author

@alxndrsn another option to prevent this at Slonik level (assuming brianc/node-postgres#3214 does not get merged), would be to auto generate statement name based on the query.

👍 but I suspect there is some extra overhead to named prepared statements vs unnamed.

@gajus
Copy link
Owner

gajus commented Jun 4, 2024

@alxndrsn I was going to update pg to allow queryMode: 'extended', but it only works when query has no parameters, so I am not sure how useful that is.

@alxndrsn
Copy link
Contributor Author

alxndrsn commented Jun 5, 2024

it only works when query has no parameters

Simple queries do not support parameters, so extended query mode is always when parameters are supplied.

@gajus
Copy link
Owner

gajus commented Jun 5, 2024

Released in v45.6.0.

Thank you!

@gajus gajus closed this as completed Jun 5, 2024
@gajus
Copy link
Owner

gajus commented Jun 5, 2024

Looks like the performance hit from enabling this is fairly substantive. Might may want to make this configurable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants