Batch errors associated with wrong statement

[phemmer]

When using SendBatch(), if one of the statements in the batch returns an error during prepared statement generation, the error is returned on the first result in the BatchResult. This is incorrect as the error may have been a result of latter statements.

For example:

batch := &pgx.Batch{}
batch.Queue("create temp table batchtest (col int)")
batch.Queue("insert into batchtest values (1)")
result := conn.SendBatch(context.Background(), batch)

_, err := result.Exec()
fmt.Print(err)

ERROR: relation "batchtest" does not exist (SQLSTATE 42P01)

This makes it very confusing as returning the error on the first result.Exec() implies the error came from create temp table ....

I'm not sure the right way this should be solved. However I would argue that if a prepared statement doesn't already exist in the cache, it should not be making a round trip to create it, as this defeats the entire point of using batch statements.

Additionally, in my use case it's actually the usage of prepared statements that is causing the problem. The second statement is throwing an error because a table being referenced doesn't exist. However the first statement in the batch is the one creating it.

[phemmer]

Another related issue, the documentation for ConnConfig states:

    // BuildStatementCache creates the stmtcache.Cache implementation for connections created with this config. Set
    // to nil to disable automatic prepared statements.

For batch statements this is incorrect. Conn.SendBatch contains the following code:

        if c.stmtcache != nil && c.stmtcache.Cap() >= len(distinctUnpreparedQueries) {
            stmtCache = c.stmtcache
        } else {
            stmtCache = stmtcache.New(c.pgConn, stmtcache.ModeDescribe, len(distinctUnpreparedQueries))
        }

So basically it ignores the setting and creates a prepared statement cache anyway.

[jackc]

pgx uses the PostgreSQL extended protocol by default. The extended protocol can only execute prepared statements -- in a sense -- there is a special nameless and ephemeral prepared statement that is used. See https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-FLOW-EXT-QUERY for the details.

When using the extended protocol pgx needs to know the underlying data types of the parameters and results. Without a statement cache it does two round trips to the database, one to prepare and describe the SQL and another to execute it. This is necessary to safely use the binary format -- which can make a significant performance improvement.

To send batched queries pgx needs to have the description for all statements before it executes any of them. It stores these in a statement cache. It tries to use the connection cache, but if that doesn't exist or is too small it creates a temporary one just for the batched query. But note that it uses the mode stmtcache.ModeDescribe, this means that no permanent prepared statements are created. It is merely using the nameless prepared statement to get the description of all distinct statements. So it's not ignoring the BuildStatementCache setting.

With regards to statement parse time errors being reported on the first result regardless of which statement was unparseable... that is pretty tricky. All the parsing occurs before any of the executing. So there are no results to report. Just a single error regardless of which query was unparseable.

[phemmer]

Ah, thanks for the info on how batch works. I was wondering if it was some requirement of the protocol, but was having trouble finding info on the subject.

With regards to statement parse time errors being reported on the first result regardless of which statement was unparseable... that is pretty tricky. All the parsing occurs before any of the executing. So there are no results to report. Just a single error regardless of which query was unparseable.

From a quick skim through the docs & code, it seems that there are no server-side resources that are held when using ModeDescribe. So what if pgx.Batch held a reference to the connection or pool, and the describe operation was performed at the time of .Queue(), where .Queue() would return an error.

The only other possibility that springs to mind is if either SendBatch() returned an error, or if .BatchResults had a .Error attribute, and perhaps a reference to or index of the queued query. This would make it clear that the batch never made it to the execution phase, and would break the artificial link between the error an the first queued query.

In any case, I think it would be good to add to the documentation on Batch that if a query doesn't exist in the cache, it will result in an additional round trip.

[jackc]

From a quick skim through the docs & code, it seems that there are no server-side resources that are held when using ModeDescribe. So what if pgx.Batch held a reference to the connection or pool, and the describe operation was performed at the time of .Queue(), where .Queue() would return an error.

I'm very reluctant to change the interface without a major version release.

The only other possibility that springs to mind is if either SendBatch() returned an error, or if .BatchResults had a .Error attribute, and perhaps a reference to or index of the queued query. This would make it clear that the batch never made it to the execution phase, and would break the artificial link between the error an the first queued query.

Potentially same issue here. Changing the interface is undesirable, though perhaps it could be done in a way that did not adversely affect existing consumers of this API who didn't also check this new error mode.

In any case, I think it would be good to add to the documentation on Batch that if a query doesn't exist in the cache, it will result in an additional round trip.

That would be fair. Though it is an implementation detail that I don't want to commit to, the only guarantee regarding this I want to make is that when executing multiple queries it will make less round trips than executing the queries individually.

[sweatybridge]

Hi @jackc , thanks for making this library open source. May I check with you what's the expected behaviour when both statements in the batch are valid? For eg.

batch := &pgx.Batch{}
batch.Queue("CREATE TABLE batchtest (col int)")
batch.Queue("INSERT INTO batchtest VALUES (1)")
result := conn.SendBatch(context.Background(), batch)

_, err := result.Exec()
fmt.Print(err)

The code above gives me an error from statement cache, but not if I execute them one by one.

ERROR: relation "batchtest" does not exist (SQLSTATE 42P01)

[jackc]

@sweatybridge Same issue as above. All statements are prepared before any statements are executed. Preparing INSERT INTO batchtest VALUES (1) fails because batchtest doesn't exist yet.

[yevgenypats]

Take a look at https://github.com/jackc/pgx/pull/1441 as it might help and also if it is on Postgres side you can convert the error to var pgErr *pgconn.PgError and check TableName to give you a hint on which table it failed on. We did something similar here - https://github.com/cloudquery/cloudquery/blob/main/plugins/destination/postgresql/client/write.go#L43

[makalaaneesh]

Hi @jackc ! We recently ran into this error. I went digging into conn.go and found that the exact error is thrown at https://github.com/jackc/pgx/blob/2ec900454bfe65daa9648488e93f7627c26b810c/conn.go#L1158 . I believe something similar to #1441 , where we include the culprit SQL statement in the error that is thrown, will be useful in this scenario as well.

Let me know if this makes sense, and if there are potentially other scenarios where we might need to include the SQL statement in the error; I can raise a PR.

[jackc]

@makalaaneesh If it can be reliably done -- sure.