update schema.graphql with denormalized schema #132

MarioBassem · 2023-09-13T11:33:57Z

Description:

this pr tries to denormalize schema generated by the processor

Changes

changed graphql.schema file with the new proposed schema
regenerated ts models
added a new db migration
updated processor mappings to handle new schema changes

Related issues:

denormalize graphql schema #134

Signed-off-by: mariobassem <[email protected]>

sameh-farouk · 2023-10-11T11:15:20Z

db/migrations/1694605589133-denormalize_data.js

+        await db.query(`CREATE TABLE "contract_resources" ("id" character varying NOT NULL, "hru" numeric NOT NULL, "sru" numeric NOT NULL, "cru" numeric NOT NULL, "mru" numeric NOT NULL, "contract_id" character varying, CONSTRAINT "PK_557de19994fcca90916e8c6582f" PRIMARY KEY ("id"))`)
+        await db.query(`ALTER TABLE "contract_resources" ADD CONSTRAINT "FK_621238dffde9099b2233650235d" FOREIGN KEY ("contract_id") REFERENCES "node_contract"("id") ON DELETE NO ACTION ON UPDATE NO ACTION`)
+    }
+}


Thanks, Mario, for all the good work!

Since this PR is about performance, have we conducted any tests or benchmarks to demonstrate the effectiveness of the performance improvements made here?
I would like to see some numbers before and after the changes for the queries that were used in production. This will help us understand the impact of the changes and whether they are worth implementing.

Also, I was wondering if there are any other parts of the system that use this database directly besides gridproxy. If so, it might be helpful to include a note in the PR description about what will be affected by this change.

sameh-farouk · 2023-10-11T11:44:16Z

src/mappings/contracts.ts

+    ip.contractId = null
+    const farm = await ctx.store.get(Farm, { where: { farmID: ip.farm.farmID } })
+    if (farm) {
+      farm.freeIps += 1


I would like to understand the motivation behind updating the counters for free and total IPs in multiple places within the code instead of relying on the database count function. I’m afraid we are sacrificing consistency for small or premature performance optimization.

In my opinion, this approach may be more error-prone, and we can end up with incorrect counters in the database.

If we really can't just use db count function, we can consider using an indexed view to maintain the IP counters.

it's pretty common, i think, that a farms is filtered by how many public ips it has, and currently, farm queries in gridproxy counts all farm ips, and counts all farm ips that don't have a contract id (this is done per farm). and since the processor actually process's the events that manipulate ips, i thought it would be more efficient to have the ip counts pre-calculated.

i don't really see how this is error-prone, can you explain?

sameh-farouk · 2023-10-11T12:06:59Z

src/mappings/contracts.ts

+    if (!savedNode) return
+    savedNode.freeMRU += savedContract.usedMRU || BigInt(0)
+    savedNode.freeSRU += savedContract.usedSRU || BigInt(0)
+    savedNode.freeHRU += savedContract.usedHRU || BigInt(0)


I keep seeing this pattern in the code where we first read the data in memory, modify it, and then write it back to the database. I’m not an expert on TypeORM, but in general, I think there are better ways to update a field or a few fields in one query. Doing so in multiple queries is less efficient and susceptible to overwrite problems (not atomic).

I never used typeORM but doing a quick search and checking repository API, I believe its safer to use .update() or .increment() and .decrement().
Another alternative is doing so using QueryBuilder.

sameh-farouk · 2023-10-11T12:25:41Z

src/mappings/contracts.ts


  if (!savedContract) return

  savedContract.state = ContractState.Deleted

-  await ctx.store.save<RentContract>(savedContract)
+  await ctx.store.save<Contract>(savedContract)


as the previous comment, can we do so in one query?

sameh-farouk · 2023-10-11T12:25:50Z

src/mappings/contracts.ts

+    if (savedNode) {
+      savedNode.dedicated = savedNode.extraFee !== null
+      savedNode.rentedBy = null
+      await ctx.store.save<Node>(savedNode)


as the previous comment, can we do so in one query?

sameh-farouk · 2023-10-11T12:28:20Z

src/mappings/contracts.ts

+    savedNode.freeHRU += (savedContract.usedHRU || BigInt(0)) - usedResources.used.hru
+    savedNode.freeSRU += (savedContract.usedSRU || BigInt(0)) - usedResources.used.sru
+
+    await ctx.store.save<Node>(savedNode)


As previous comment, should be safer to use .increment() or .decrement(). check here

sameh-farouk · 2023-10-11T12:36:42Z

src/mappings/farms.ts

@@ -70,10 +72,14 @@ export async function farmStored(
    newIP.farm = newFarm

    newFarm.publicIPs?.push(newIP)
-
+    newFarm.totalIps += 1


This could be more error-prone, and susceptible to overwrite problems (not atomic).
As explained before, i would suggest relay on db count function whenever we need such info, or consider using an indexed view for IPs counters.

sameh-farouk · 2023-10-11T12:39:55Z

src/mappings/farms.ts

    if (ip.ip.toString().indexOf('\x00') >= 0) {
      return
    }
    const savedIP = await ctx.store.get(PublicIp, { where: { ip: ip.ip.toString() }, relations: { farm: true } })
    // ip is already there in storage, don't save it again
+
+    savedFarm.totalIps += 1


As previous comment
Also, do we need to increment totalIps here?

sameh-farouk · 2023-10-11T12:43:45Z

src/mappings/contracts.ts

-    savedNode.dedicated = savedNode.extraFee !== null
-    await ctx.store.save<Node>(savedNode)
+  if (savedContract.nodeID) {
+    const savedNode = await ctx.store.get(Node, { where: { nodeID: savedContract.nodeID }, relations: { location: true, interfaces: true } })


Do we need to load these relations?

sameh-farouk · 2023-10-11T12:45:58Z

src/mappings/nodes.ts

@@ -15,6 +15,9 @@ import { Ctx } from '../processor'
 import assert from "assert";
 import { allowedNodeEnvironmentFlags } from "process";

+const ZOSUsedSRU = 107374182400;
+const ZOSMinUsedMemory = 2147483648;


What are these numbers and where it came from?
Please add a code comment explaining these magic numbers.

sameh-farouk · 2023-10-11T13:17:10Z

src/mappings/contracts.ts

+  let savedPublicIPs: PublicIp[] = await ctx.store.find(PublicIp, { where: { contractId: contractID }, relations: { farm: true } })
+  Promise.all(savedPublicIPs.map(async (ip) => {
+    ip.contractId = null
+    const farm = await ctx.store.get(Farm, { where: { farmID: ip.farm.farmID } })


I'm not sure, but do we need to read Farm instance again? can we use ip.farm directly instead?

MarioBassem added 5 commits September 13, 2023 14:31

update schema.graphql with denormalized schema

47c15b2

Signed-off-by: mariobassem <[email protected]>

update auto-generated types

7333d01

Signed-off-by: mariobassem <[email protected]>

update type mappings

e35497f

Signed-off-by: mariobassem <[email protected]>

update db migrations

5ca1f5a

Signed-off-by: mariobassem <[email protected]>

fix farm free ips & node free resources calculations

4ca6b15

Signed-off-by: mariobassem <[email protected]>

sameh-farouk requested changes Oct 11, 2023

View reviewed changes

sameh-farouk reviewed Oct 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update schema.graphql with denormalized schema #132

update schema.graphql with denormalized schema #132

MarioBassem commented Sep 13, 2023 •

edited

Loading

sameh-farouk Oct 11, 2023

sameh-farouk Oct 11, 2023 •

edited

Loading

MarioBassem Oct 11, 2023

sameh-farouk Oct 11, 2023 •

edited

Loading

sameh-farouk Oct 11, 2023

sameh-farouk Oct 11, 2023

sameh-farouk Oct 11, 2023

sameh-farouk Oct 11, 2023

sameh-farouk Oct 11, 2023

sameh-farouk Oct 11, 2023

sameh-farouk Oct 11, 2023

sameh-farouk Oct 11, 2023

update schema.graphql with denormalized schema #132

Are you sure you want to change the base?

update schema.graphql with denormalized schema #132

Conversation

MarioBassem commented Sep 13, 2023 • edited Loading

Description:

Changes

Related issues:

Choose a reason for hiding this comment

sameh-farouk Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameh-farouk Oct 11, 2023 • 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

MarioBassem commented Sep 13, 2023 •

edited

Loading

sameh-farouk Oct 11, 2023 •

edited

Loading

sameh-farouk Oct 11, 2023 •

edited

Loading