eclipse-vertx / vertx-sql-client

High performance reactive SQL Client written in Java
Apache License 2.0
894 stars 200 forks source link

[Postgres] "insert into ... returning id" doesn't return all generated keys when executed in a batch, but returns just 1 key/row #1439

Closed salmonb closed 6 months ago

salmonb commented 6 months ago

Version

Vert.x version 4.5.7 (I also tried 4.5.8 and have the same issue)

Context

The returned RowSet should have the same number of rows as the number of tuples in List<Tuple> passed to executeBatch(), but it has only 1 row, which prevents us to get other generated keys.

Do you have a reproducer?

https://github.com/salmonb/vertx-pgreturning-issue

Steps to reproduce

See README of the reproducer

Extra

I'm on macOS Big Sur & Postgres 14.12, but also reproduced the issue on Ubuntu 22.04

tsegismont commented 6 months ago

When the query has a returning clause and a batch is executed, there should be as many RowSet returned as batches submitted.

Try this:

client
  .preparedQuery("insert into person (first_name, last_name) values ($1, $2) returning id")
  .executeBatch(batch)
  .onSuccess(res -> {
    for (RowSet<Row> rows = res;rows.next() != null;rows = rows.next()) {
      Integer personId = rows.iterator().next().getInteger("id");
      System.out.println("generated key: " + personId);
    }
  });

See https://vertx.io/docs/vertx-pg-client/java/#_returning_clauses

salmonb commented 6 months ago

Thanks Thomas, but I already contacted the chat support on Discord before opening this issue, and they already asked me to do this, which doesn't help (I still have the issue).

In addition, this code is wrong because it's calling rows.next() twice on each iteration instead of once. The correct code should be:

client
  .preparedQuery("insert into person (first_name, last_name) values ($1, $2) returning id")
  .executeBatch(batch)
  .onSuccess(res -> {
    for (RowSet<Row> rows = res; rows != null;rows = rows.next()) {
      Integer personId = rows.iterator().next().getInteger("id");
      System.out.println("generated key: " + personId);
    }
  });

which can be simplified to

client
  .preparedQuery("insert into person (first_name, last_name) values ($1, $2) returning id")
  .executeBatch(batch)
  .onSuccess(res -> {
    for (Row row : res) {
      Integer personId = row.getInteger("id");
      System.out.println("generated key: " + personId);
    }
  });

which is basically what I'm doing in my reproducer.

Have you cloned and tried my reproducer?

tsegismont commented 6 months ago

Indeed, the example from the doc is wrong. It should be as you wrote:

client
  .preparedQuery("insert into person (first_name, last_name) values ($1, $2) returning id")
  .executeBatch(batch)
  .onSuccess(res -> {
    for (RowSet<Row> rows = res; rows != null;rows = rows.next()) {
      Integer personId = rows.iterator().next().getInteger("id");
      System.out.println("generated key: " + personId);
    }
  });

Yet it is not equivalent to:

client
  .preparedQuery("insert into person (first_name, last_name) values ($1, $2) returning id")
  .executeBatch(batch)
  .onSuccess(res -> {
    for (Row row : res) {
      Integer personId = row.getInteger("id");
      System.out.println("generated key: " + personId);
    }
  });

In the first case, the code iterates over all the RowSet instances returned, which all contain a single Row. In the second case, the code iterates over all rows of the first RowSet, which contain a single Row.

So please update this part of the reproducer: https://github.com/salmonb/vertx-pgreturning-issue/blob/89e7f2f914d609c629bab43f0252924f170861b2/src/main/java/dev/salmonb/vertx/issue/pgreturning/PgReturningIssueVerticle.java#L48-L50

And, if the problem persists, we'll explore new ideas.

tsegismont commented 6 months ago

One more thing, this code:

                    if (rs.size() == batch.size())
                        System.out.println("Correct RowSet size");
                    else
                        System.out.println("Incorrect RowSet size: " + rs.size() + " instead of " + batch.size());

Will always print "Incorrect RowSet size", for the reason explained in the previous comment. When combining RETURNING with batch execution, you get one RowSet for each element of the batch (here a single insert row id).

salmonb commented 5 months ago

Thanks for this insight. I was confused with the RowSet API and thought it was a simple Set of Row, but it's not exactly.

I think my code is now correct:

                    int totalRowCount = 0;
                    for (RowSet<Row> rows = rs; rows != null; rows = rows.next()) {
                        totalRowCount += rows.size();
                        for (Row row : rows) {
                            Object personId = row.getValue(0);
                            System.out.println("generated key: " + personId);
                        }
                    }
                    if (totalRowCount == batch.size())
                        System.out.println("Correct RowSet size");
                    else
                        System.out.println("Incorrect RowSet size: " + totalRowCount + " instead of " + batch.size());

Anyway, at least this issue had the benefit to fix the Vert.x doc.

tsegismont commented 5 months ago

Anyway, at least this issue had the benefit to fix the Vert.x doc.

This is a great benefit and thank you for reporting an issue. Please do it again if you find other problems.