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

fix(pgwire): fix spurious error when executing "create table" SQL from Rust #2385

Merged
merged 17 commits into from Aug 5, 2022

Conversation

bluestreak01
Copy link
Member

@bluestreak01 bluestreak01 commented Aug 1, 2022

When executing any CREATE TABLE statement via Rust table would be created but Rust client will receive spurious "table already exists" error. This PR fixes that by not re-executing CREATE TABLE despite Rust treating it as a prepared statement.

Fixes the following:

  • in "simple" mode we print millis using S format, which removes leading zeroes. This causes incorrect data values to be returned and inconsistent behaviour between "simple" and "extended" protocols
  • in "extended" protocol mode we could run non-renetrable SQL statements twice, those include CREATE TABLE, CREATE AS SELECT and COPY
  • errors parsing BIND message would leave syncActions array dirty. Subsequent SQL execution crashes JDBC driver due to duplicate response from the server
  • we force disconnect client if they attempt to send CLOSE message on non-existing statement. This behaviour is removed. Some PARSE errors could end up never registering named statement, which would subsequently fail to close.

Adds tests, most PG wire tests, which require simple connection, are now run by a wrapper. This wrapper executes the same "lambda" against 12 different protocol combinations. This ensures consistent behaviour wherever possible.

Gotcha

testBatchInsertWithTransaction excludes simple protocol due to a bug. With simple protocol enabled, test hangs the connection on "cannot insert records out of order" error. We are sending invalid error response to the driver. This requires wireshark investigation as the behaviour is specific to batch execution over simple protocol. @bziobrowski would you mind having a look please?

@bziobrowski
Copy link
Contributor

The real issue is that create table statement is executed in parse phase instead of execute .
First test below reproduces the issue and should be more readable than hex . It does work with the fix but if CTAS statement contains bind variable it still returns error even though in the background it creates table and populates with bad data (first column is all nulls because bind variable value is missing during execution).

    @Test
    public void testCreateTable() throws Exception {
        assertMemoryLeak(() -> {
            try (
                    final PGWireServer ignored = createPGServer(2);
                    final Connection connection = getConnection(Mode.ExtendedForPrepared, false, -1)
            ) {
                connection.setAutoCommit(false);
                try (PreparedStatement pstmt = connection.prepareStatement("create table t (\n" +
                        "  a SYMBOL,\n" +
                        "  b TIMESTAMP)\n" +
                        "    timestamp(b)")) {
                    pstmt.execute();
                }
            }
        });
    }

    @Test
    public void testCreateTable2() throws Exception {
        assertMemoryLeak(() -> {
            try (
                    final PGWireServer ignored = createPGServer(2);
                    final Connection connection = getConnection(Mode.ExtendedForPrepared, false, -1)
            ) {
                connection.setAutoCommit(false);
                try (PreparedStatement pstmt = connection.prepareStatement("create table t as " +
                        "(select cast(x + ? as long) a, cast(x as timestamp) b from long_sequence(10))")) {
                    pstmt.setLong(1, 10);
                    pstmt.execute();
                }
            }
        });
    }

@bluestreak01
Copy link
Member Author

Thank you @bziobrowski Your test is a lot better and reproduces the very same issue found by Rust.

Regarding bind variables in CREATE TABLE. This is not so much of PG wire issue. We do not have infrastructure at compiler level to execute this. That said, it is not a difficult addition but use-case seems a bit far fetched for now. I would not bother in this PR.

@bziobrowski
Copy link
Contributor

CTAS executed as prepared statements fail even when there are no actual bind variables so this fix works only for CREATE TABLE x (spec) statements .

@bluestreak01 bluestreak01 marked this pull request as draft August 2, 2022 18:51
@bluestreak01
Copy link
Member Author

All the issues Bolek mentioned are fixed.

After merging master, which brought copy unification PR new “fuzz” PG tests began to fail specifically for “copy” sql. I asked @puzpuzpuz to take a look asap

@puzpuzpuz
Copy link
Contributor

The COPY issue is now fixed as well as the CTAS/DDL not being executed on multiple statement runs.

@bluestreak01 bluestreak01 marked this pull request as ready for review August 4, 2022 18:27
@ideoma
Copy link
Collaborator

ideoma commented Aug 4, 2022

[PR Coverage check]

😍 pass : 85 / 87 (97.70%)

file detail

path covered line new line coverage
🔵 io/questdb/griffin/SqlCompiler.java 36 38 94.74%
🔵 io/questdb/cutlass/pgwire/PGWireServer.java 2 2 100.00%
🔵 io/questdb/cutlass/pgwire/PGConnectionContext.java 13 13 100.00%
🔵 io/questdb/std/datetime/millitime/DateFormatUtils.java 1 1 100.00%
🔵 io/questdb/griffin/engine/ops/CopyFactory.java 33 33 100.00%

@puzpuzpuz puzpuzpuz added Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution labels Aug 5, 2022
@puzpuzpuz puzpuzpuz self-requested a review August 5, 2022 06:59
@bluestreak01 bluestreak01 merged commit ac62136 into master Aug 5, 2022
@bluestreak01 bluestreak01 deleted the vi_rust_ddl branch August 5, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants