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

Inconsistent Parameter Handling Between QueryExecModeCacheStatement and QueryExecModeSimpleProtocol #2157

Open
suleimi opened this issue Oct 28, 2024 · 1 comment
Labels

Comments

@suleimi
Copy link

suleimi commented Oct 28, 2024

I'm not sure if this is a bug or more just a consequence of how parameter handling is implemented in the pgx library, especially when using custom types. But i've just noticed that using QueryExecModeCacheStatement vs QueryExecModeSimpleProtocol when building and executing a psql query results in two different sql queries, (and in my case one of which is wrong and causes an error):

To Reproduce

package main

import (
	"context"
	"log"

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

type SomeFancyNumber int

const (
	FancyNumber1 SomeFancyNumber = iota
	FancyNumber2
)

func (s SomeFancyNumber) String() string {
	switch s {
	case FancyNumber1:
		return "fancy_number_1"
	case FancyNumber2:
		return "fancy_number_2"
	}
	return "unknown"
}

func main() {
	simpleConn, err := pgx.Connect(context.Background(), "host=postgres port=5432 user=auser password=apassword dbname=adbname sslmode=disable default_query_exec_mode=simple_protocol")
	if err != nil {
		log.Fatal(err)
	}
	defer simpleConn.Close(context.Background())

	cacheStatementConn, err := pgx.Connect(context.Background(), "host=postgres port=5432 user=auser password=apassword dbname=adbname sslmode=disable default_query_exec_mode=cache_statement")
	if err != nil {
		log.Fatal(err)
	}
	defer cacheStatementConn.Close(context.Background())

	// Create a table and put one record in it
	//
	_, err = simpleConn.Exec(context.Background(), `CREATE TABLE IF NOT EXISTS test (id SERIAL PRIMARY KEY, anumber INT NOT NULL);`)
	if err != nil {
		log.Fatalf("Failed to create table: %v", err)
	}
	log.Println("Table 'test' created successfully!")

	_, err = simpleConn.Exec(context.Background(), `INSERT INTO test (anumber) VALUES ($1);`, FancyNumber1) // Replace 42 with the desired value
	if err != nil {
		log.Printf("Simple protocol exec resulted in error %v", err)
	} else {
		log.Println("Record inserted successfully into 'test' table using simple protocol mode!")
	}

	_, err = cacheStatementConn.Exec(context.Background(), `INSERT INTO test (anumber) VALUES ($1);`, FancyNumber1) // Replace 42 with the desired value
	if err != nil {
		log.Printf("Cache statement exec resulted in error %v", err)
	} else {
		log.Println("Record inserted successfully into 'test' table using cache statement mode!")
	}
}

Expected behavior

identical or equivalent resolved queries irregardless of the execution modes of simple_protocol and cache_statement

i.e a log entry of both

Record inserted successfully into 'test' table using simple protocol mode!
Record inserted successfully into 'test' table using cache statement mode!

Actual behavior

The queries are resolved differently for the different execution modes.

For default_query_exec_mode=simple_protocol; the query resolves to:

INSERT INTO test (anumber) VALUES ("fancy_number_1");

For default_query_exec_mode=cache_statement; the query resolves to:

INSERT INTO test (anumber) VALUES (0);

Version

  • Go: go1.22 darwin/amd64
  • PostgreSQL: container image: postgres:16-alpine
  • pgx: v5.7.1

Additional context

This can be worked around by explicitly converting SomeFancyNumber to an int before passing it to the Exec function. While this may not a bug in the strictest sense, it does represent a design quirk in the library that may be helpful to document in the pgx library usage guidelines. Users switching modes between cache_statement and simple_protocol might not expect that some of their queries need to be tweaked/refactor.

@suleimi suleimi added the bug label Oct 28, 2024
jackc added a commit that referenced this issue Oct 30, 2024
@jackc
Copy link
Owner

jackc commented Oct 30, 2024

This behavior is undesirable, but I believe it is ultimately unavoidable unless you can give pgx some help.

When using QueryExecModeCacheStatement, pgx knows that the desired type for $1 is a PostgreSQL int4. It tries to find a path to a binary encoded PostgreSQL int4 and finds it by converting SomeFancyNumber to its underlying Go int type.

When using QueryExecModeSimpleProtocol pgx can only use the text protocol and it doesn't know what the PostgreSQL type is. You can let pgx know the desired type with:

simpleConn.TypeMap().RegisterDefaultPgType(SomeFancyNumber(0), "int4")

Unfortunately, that is not quite enough. pgx tries to convert SomeFancyNumber to directly to a PostgreSQL int4 first, but it can't. As mentioned above, the simple protocol must use the text protocol. So pgx tries to convert SomeFancyNumber to a string which it can because SomeFancyNumber implements fmt.Stringer. pgx finds that path to encode the value before it finds the path of converting SomeFancyNumber to its underlying Go int type.

What does solve the problem is also implementing pgtype.Int64Valuer. That path is found before the fmt.Stringer path.

func (s SomeFancyNumber) Int64Value() (pgtype.Int8, error) {
	return pgtype.Int8{Int64: int64(s), Valid: true}, nil
}

I updated the documentation to clarify this in c76a650.

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

No branches or pull requests

2 participants