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

Pagination & transaction #1278

Closed
Dafyh opened this issue Oct 16, 2024 · 2 comments
Closed

Pagination & transaction #1278

Dafyh opened this issue Oct 16, 2024 · 2 comments

Comments

@Dafyh
Copy link

Dafyh commented Oct 16, 2024

Hello 👋,

I notice an issue when I want to use pagination (offset + limit) and then perform transactions.
Here is an example that reproduces the problem:

const sqlite = require("better-sqlite3");
const path = require("node:path");
const fs = require("node:fs/promises")
const crypto = require("node:crypto")

function* getResults(db) {
  let offset = 0;
  const limit = 10;

  while (true) {
    const results = db
      .prepare(
        `SELECT id FROM bar WHERE foo IS NULL ORDER BY id LIMIT ${limit} OFFSET ${offset}`
      )
      .all();

    if (results.length === 0) {
      break;
    }

    offset += limit;
    yield results;
  }
}

function createDb() {
  const db = sqlite(path.join(process.cwd(), "database.db"));
  db.exec(`
    CREATE TABLE bar (
      id INTEGER PRIMARY KEY AUTOINCREMENT,
      foo TEXT DEFAULT NULL
    );
  `);

  const transaction = db.transaction(() => {
    Array(100).fill(null).forEach(() => {
      db.prepare("INSERT INTO bar (foo) VALUES (NULL)").run();
    });
  });

  transaction();

  return db;
}

async function run() {
  try {
    await fs.unlink(path.join(process.cwd(), "database.db"));
  } catch {
    // Do nothing
  }

  const db = createDb();

  let ids = [];
  for (const results of getResults(db)) {
    ids = [...ids, ...results.map(({ id }) => id)]

    const transaction = db.transaction((ids) => {
      ids.forEach((id) => {
        db.prepare(`UPDATE bar SET foo = ? WHERE id = ?`).run(crypto.randomBytes(6).toString("hex"), id);
      });
    });

    transaction(ids);
  }

  console.log(ids)
  /*
  [
    1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 21,
    22, 23, 24, 25, 26, 27, 28, 29, 30, 41, 42,
    43, 44, 45, 46, 47, 48, 49, 50, 61, 62, 63,
    64, 65, 66, 67, 68, 69, 70, 81, 82, 83, 84,
    85, 86, 87, 88, 89, 90
  ]

  Without transaction :
  [
    1,  2,  3,   4,  5,  6,  7,  8,  9, 10, 11, 12,
    13, 14, 15,  16, 17, 18, 19, 20, 21, 22, 23, 24,
    25, 26, 27,  28, 29, 30, 31, 32, 33, 34, 35, 36,
    37, 38, 39,  40, 41, 42, 43, 44, 45, 46, 47, 48,
    49, 50, 51,  52, 53, 54, 55, 56, 57, 58, 59, 60,
    61, 62, 63,  64, 65, 66, 67, 68, 69, 70, 71, 72,
    73, 74, 75,  76, 77, 78, 79, 80, 81, 82, 83, 84,
    85, 86, 87,  88, 89, 90, 91, 92, 93, 94, 95, 96,
    97, 98, 99, 100
  ]
  */
}

run();

We can see that the offset correctly returns the first 10, then skips the next 10. Without the transaction to update the rows, the behavior is correct; I get 1 to 10, then 11 to 20, and so on.

Is this normal behavior, or is there indeed an issue?

Thank you 😊.

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Oct 16, 2024

I don't know what you are trying to achieve, but this has nothing to do with transactions. You are updating rows, which affects the query in getResults (foo IS NULL no longer matches). And due to the way you update the list ids = [...ids, ...results.map(({ id }) => id)] you end up with a weird pattern.

If you log what you're doing you'll see that it doesn't make sense:

+console.log('update', id);
db.prepare(`UPDATE bar SET foo = ? WHERE id = ?`).run(crypto.randomBytes(6).toString("hex"), id);

results

update 1
update 2
update 3
update 4
update 5
update 6
update 7
update 8
update 9
update 10
update 1
update 2
update 3
update 4
update 5
update 6
update 7
update 8
update 9
update 10
update 21
update 22
update 23
update 24
update 25
update 26
update 27
update 28
update 29
update 30
update 1
update 2
update 3
update 4
update 5
update 6
update 7
update 8
update 9
update 10
update 21
update 22
update 23
update 24
update 25
update 26
update 27
update 28
update 29
update 30
update 41
update 42
update 43
update 44
update 45
update 46
update 47
update 48
update 49
update 50
update 1
update 2
update 3
update 4
update 5
update 6
update 7
update 8
update 9
update 10
update 21
update 22
update 23
update 24
update 25
update 26
update 27
update 28
update 29
update 30
update 41
update 42
update 43
update 44
update 45
update 46
update 47
update 48
update 49
update 50
update 61
update 62
update 63
update 64
update 65
update 66
update 67
update 68
update 69
update 70
update 1
update 2
update 3
update 4
update 5
update 6
update 7
update 8
update 9
update 10
update 21
update 22
update 23
update 24
update 25
update 26
update 27
update 28
update 29
update 30
update 41
update 42
update 43
update 44
update 45
update 46
update 47
update 48
update 49
update 50
update 61
update 62
update 63
update 64
update 65
update 66
update 67
update 68
update 69
update 70
update 81
update 82
update 83
update 84
update 85
update 86
update 87
update 88
update 89
update 90

You're essentially removing 10 items from the results (by making foo not null) while also moving offset further, which will then cause 10 items to be skipped.

@Dafyh
Copy link
Author

Dafyh commented Oct 16, 2024

Thank you for your response.

The logs were included for the example, but indeed the example was incorrect. I intended to pass results into the transaction function, not ids.

However, my mind clicked when I read your last sentence. Indeed, the cursor shifts with the foo IS NULL condition, hence my mistake.

I simply removed the offset to resolve this issue.

Thank you.

@Dafyh Dafyh closed this as completed Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants