jackc / pgx

PostgreSQL driver and toolkit for Go
MIT License
10.83k stars 845 forks source link

`EXPLAIN (GENERIC_PLAN)` does not correctly handle 0 args #2133

Open nightmarlin-dojo opened 1 month ago

nightmarlin-dojo commented 1 month ago

Describe the bug

When using EXPLAIN (GENERIC_PLAN) on a PG16 database, pgx incorrectly tries to assign values from the args to Query to the query parameters. In this scenario it should probably error if any args are provided instead.

To Reproduce

Expected behavior

I expected pgx to recognise that explain (generic_plan) does not require arguments to be provided, accept the query, and return the query plan provided by the database

PSQL correctly returns the query plan as expected

$ psql --no-psqlrc --tuples-only -c 'explain (generic_plan) select items.name from items where items.item_id = $1'
Index Scan using items_pkey on items  (cost=0.15..8.17 rows=1 width=32)
  Index Cond: (item_id = $1)

Actual behavior

pgx returns an error, as no arguments were provided to the query.

expected 1 arguments, got 0

Version

Additional context

jackc commented 1 month ago

By default, pgx uses the extended protocol in which there are separate steps for parsing, describing, and executing the statement.

It appears that PostgreSQL is parsing the explain statement as if the $1 is actually a required argument. You can see this by using PgConn().Prepare()

// main.go
package main

import (
    "context"
    "fmt"
    "log"
    "os"
    "os/signal"

    "github.com/jackc/pgx/v5"
)

func main() {
    ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt)
    defer cancel()

    conn, err := pgx.Connect(ctx, os.Getenv("DATABASE_URL"))
    if err != nil {
        log.Fatal("opening db connection: ", err)
    }
    defer func() { _ = conn.Close(ctx) }()

    sd, err := conn.PgConn().Prepare(ctx, "test", `explain (generic_plan) select items.name from items where items.item_id = $1`, nil)
    if err != nil {
        log.Fatal("error preparing: ", err)
    }

    fmt.Println(sd.ParamOIDs)
}

Output:

[23]

So PostgreSQL is reporting that the statement explain (generic_plan) select items.name from items where items.item_id = $1 requires one integer param. That looks like a PostgreSQL bug to me. pgx won't execute the statement without the param that PostgreSQL says it requires.

In theory, you should be able to pass pgx.QueryExecModeSimpleProtocol to QueryRow() to force it to use the simple protocol. Unfortunately, when using the simple protocol, pgx has to parse the SQL statement to find dollar variables and safely interpolate the values. So it refuses to execute as well because it also sees the $1 as an actual required variable. 🤦

So ultimately, to get this to work you would need to drop down to the pgconn layer.

    results, err := conn.PgConn().Exec(
        ctx,
        `explain (generic_plan) select items.name from items where items.item_id = $1`,
    ).ReadAll()
    if err != nil {
        log.Fatal("executing query: ", err)
    }

    for _, result := range results {
        for _, row := range result.Rows {
            srow := make([]string, len(row))
            for i, v := range row {
                srow[i] = string(v)
            }

            fmt.Println(srow)
        }
    }
[Index Scan using items_pkey on items  (cost=0.15..8.17 rows=1 width=32)]
[  Index Cond: (item_id = $1)]
nightmarlin-dojo commented 1 month ago

Thanks for the prompt reply! This sounds pretty interesting - just to check I'm understanding you here:

  1. pgx tries to prepare the query

    • Not using the prepare statement, but the underlying PostgreSQL frontend/backend message protocol
  2. PostgreSQL itself reports that the query string contains a query parameter (in this case, of type integer)

    This behaviour could be deemed either correct or incorrect, as the query string does technically contain a query parameter, but it isn't actually expected to be assigned a value.

  3. pgx then returns an error because it hasn't been provided with an argument to fill the value of that query parameter, and expects to always be passed one if one is detected

And then in pgx.QueryExecModeSimpleProtocol mode, pgx is technically correct to fail here as it mimics the same parse behaviour as the PostgreSQL server, returns that same required parameter as a result of the parse, and returns an error as before...

A fun one for sure 😆 On the upside, if it's a more general PostgreSQL bug, maybe someone else has seen it too... I'll see if I can ask about it upstream and will update this issue if I get a response - it's possible the PostgreSQL maintainers would expect clients to handle this (although I doubt it).

Many thanks for providing the workaround - it's much preferred over using a different DB driver in the one place I need to use this :D

jackc commented 1 month ago

Thanks for the prompt reply! This sounds pretty interesting - just to check I'm understanding you here:

Yes, I think you have it.