improvement: rework Store.toSQL function to minimize UPDATE statements #21

jgoux · 2024-03-06T16:41:35Z

We have a user who encountered a "bug" because we split our insertions between INSERT and UPDATE statements.

This PR's goal is to use as many INSERT statements as we can. UPDATE statements should only be used if we encounter a circular dependency.

The strategy here is to follow the rows structure in order to generate the statements.

For a given row, we look for parents, and if there are, we recursively (sorry @avallete 😂) create those parents statements.
It ensures that we will always create the rows in the right order, so we can get rid of the UPDATE.

Context: https://discord.com/channels/788353076129038346/1214886051886268426

I also updated the dependencies and rewrote some imports, I commented on the interesting files so it's easier to follow. 👍

jgoux · 2024-03-07T17:15:59Z

packages/seed/src/core/store/store.ts

@@ -18,9 +40,230 @@ export abstract class StoreBase implements Store {
    );
  }

+  protected _toSQL(props: {


This is the relevant part with the algorithm.

jgoux · 2024-03-07T17:17:21Z

packages/seed/src/dialects/postgres/store.ts

+    return this._toSQL({
+      serializeInsertStatement,
+      serializeUpdateStatement,
+      getEndStatements,
+    });


The dialect implementation is now slimmed down to serializing the insert and update statements! (+ providing optional "end statements", in Postgres' case, to fix the sequences).

jgoux · 2024-03-07T17:18:08Z

packages/seed/src/dialects/postgres/store.test.ts

@@ -253,5 +253,50 @@ describe.each(["postgresJs"] as const)("store: %s", (adapter) => {
        ]),
      );
    });
+    test("should handle circular references", async () => {


I added a simple case, but I'm sure @avallete will be able to provide more elaborated ones and break all the things! 😂

crack his fingers

justinvdm · 2024-03-11T08:18:02Z

packages/seed/src/core/codegen/userModels/generateUserModels.ts

-import { type CodegenContext } from "#core/codegen/codegen.js";
+import { type Shape, type TableShapePredictions } from "#trpc/shapes.js";
+import { type CodegenContext } from "../../codegen/codegen.js";
+import { shouldGenerateFieldValue } from "../../dataModel/shouldGenerateFieldValue.js";


@jgoux bikeshed, but just checking if these .. paths are intentional?

I think so, they live in the same "module" which is #core.

Ah, so convention-wise, we should use ../../ if its in the same "module" (e.g. code in core/ importing other code in core/, and # if its in a different module (e.g. code in dialect/ importing code in core/)?

Yes, but it's very informal, so I wouldn't obsess on it. It's just a way for me to keep layers of responsibility separated. If I see a "#" I know it's from another layer of responsibility.

avallete · 2024-03-11T08:46:47Z

packages/seed/src/core/store/store.ts

+    // do we need to keep track of the exact chain of parents?
+    if (ctx.pending[parent.type].includes(parentRowId)) {
+      if (parent.isRequired) {
+        throw new Error(


question

Should I try to add tests case on this PR so we address that before merging ?

I wonder if the fact that we don't handle circular deps will decrease the UX for most of the users or not. Because before we were just appending some update and that was it.

We don't handle non-nullable circular deps, which wouldn't be insertable anyway.
I don't think this comment is relevant anymore, I think the way we generate UPDATE statements and replace these nullable FK with NULL when necessary solves it. But you can try breaking it, of course, maybe I'm missing obvious cases!

justinvdm · 2024-03-11T10:26:19Z

packages/seed/src/core/store/store.ts

+    serializeUpdateStatement: ctx.serializeUpdateStatement,
+  });
+
+  ctx.insertStatements.push(insertStatement);


Is the idea that this line would be reached for parents first, then their children later? Such that we're always inserting parents before children, to avoid needing to later on update already inserted rows?

justinvdm · 2024-03-11T10:29:37Z

packages/seed/src/core/store/store.ts

+    const insertStatements: Array<string> = [];
+    const updateStatements: Array<string> = [];
+
+    const sortedModels = sortModels(this.dataModel);


For what reason do we topologically sort the models? To make sure the parents are reached before other ancestors, or something?

justinvdm · 2024-03-11T10:33:03Z

packages/seed/src/core/store/store.ts

+          ].join(EOL),
+        );
+      } else {
+        updatableParents.push(parent);


Would this line only be reached if there is a cyclic dependency + there is at least one nullable relation in the chain causing the cycle?

justinvdm · 2024-03-11T10:38:32Z

packages/seed/src/dialects/postgres/store.test.ts

+          foreign key (last_order_id) references "order"(id);
+
+        alter table product add constraint fk_first_order
+          foreign key (first_order_id) references "order"(id);


Is it worth also testing cycles where there are relations in-between? e.g. instead of order->customer->order and order->product->order, something like order->transaction->customer->order, or something

justinvdm

Just left a bunch of questions for my understanding, but looks great! Code makes sense (provided I understood it correctly :D but the questions should indicate that at least). Nice that we also were able to abstract out the bulk of the logic so it is not per-dialect. Great work.

jgoux · 2024-03-12T09:53:14Z

Reconverting the PR to draft as @avallete was able to destroy my implementation with more complicated tests!
I'll focus on this PR while I'm done with the CLI.
@justinvdm I'll address your comment then, as the implementation will evolve. 👍

justinvdm · 2024-03-12T11:00:28Z

Reconverting the PR to draft as @avallete was able to destroy my implementation with more complicated tests! I'll focus on this PR while I'm done with the CLI. @justinvdm I'll address your comment then, as the implementation will evolve. 👍

Cool, but was really just me wanting to understand the approach. If the implementation changing though, then I'll just re-grok and re-ask when the PR is open again, so works for me.

jgoux · 2024-04-04T07:55:16Z

I'd like us to consider using a more strategic approach:

attempt to disable FK checks (doable in both pg and sqlite)
if 1. isn't possible, using the approach in this PR
if 2. fails because of cycles maybe we could fallback on the current behaviour?

jgoux added 5 commits March 6, 2024 16:51

improvement: rework insert statements

e4937c3

tests are green

09b0c4f

remove console

2b24c78

refactor

9539182

refactor

d964120

jgoux changed the title ~~improvement: rework Store.toSQL function~~ improvement: rework Store.toSQL function to minimize UPDATE statements Mar 6, 2024

jgoux added 9 commits March 6, 2024 17:48

context in error

3f81cfd

wip

c3d9172

it works

74e291a

use indexes directly for rowIds

c88d833

refactor

d8d8723

refactor

09f5216

Merge branch 'main' into jgoux/statements-no-update

2842e2b

cleanup paths

a15b277

bump deps

4b07a9c

jgoux marked this pull request as ready for review March 7, 2024 17:04

jgoux commented Mar 7, 2024

View reviewed changes

jgoux requested review from justinvdm and avallete March 11, 2024 08:12

justinvdm reviewed Mar 11, 2024

View reviewed changes

avallete reviewed Mar 11, 2024

View reviewed changes

avallete added 2 commits March 11, 2024 11:00

wip: add complex circular deps test case

Loading
Loading status checks…

780f048

wip: add test for non-nullable circ deps error

Loading
Loading status checks…

3abb4f8

justinvdm reviewed Mar 11, 2024

View reviewed changes

justinvdm self-requested a review March 11, 2024 10:40

justinvdm approved these changes Mar 11, 2024

View reviewed changes

avallete added 2 commits March 11, 2024 11:56

wip: add larger circular deps schema

Loading
Loading status checks…

37ba696

wip: add more tests

Loading
Loading status checks…

15c7319

jgoux marked this pull request as draft March 12, 2024 09:52

This was referenced Mar 13, 2024

feat(core): path connection strategy #32

Merged

feat: add e2e tests for cyclic deps #45

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: rework Store.toSQL function to minimize UPDATE statements #21

improvement: rework Store.toSQL function to minimize UPDATE statements #21

jgoux commented Mar 6, 2024 •

edited

Loading

jgoux Mar 7, 2024

jgoux Mar 7, 2024

jgoux Mar 7, 2024

avallete Mar 7, 2024

justinvdm Mar 11, 2024

jgoux Mar 11, 2024

justinvdm Mar 11, 2024

jgoux Mar 11, 2024

avallete Mar 11, 2024

jgoux Mar 11, 2024

justinvdm Mar 11, 2024 •

edited

Loading

justinvdm Mar 11, 2024

justinvdm Mar 11, 2024 •

edited

Loading

justinvdm Mar 11, 2024

justinvdm left a comment

jgoux commented Mar 12, 2024

justinvdm commented Mar 12, 2024

jgoux commented Apr 4, 2024

improvement: rework Store.toSQL function to minimize UPDATE statements #21

Are you sure you want to change the base?

improvement: rework Store.toSQL function to minimize UPDATE statements #21

Conversation

jgoux commented Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinvdm Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinvdm Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinvdm left a comment

Choose a reason for hiding this comment

jgoux commented Mar 12, 2024

justinvdm commented Mar 12, 2024

jgoux commented Apr 4, 2024

jgoux commented Mar 6, 2024 •

edited

Loading

justinvdm Mar 11, 2024 •

edited

Loading

justinvdm Mar 11, 2024 •

edited

Loading