Skip to content

Conversation

@dkijania
Copy link
Member

@dkijania dkijania commented Mar 12, 2025

Reimplementing caqti upgrade after revertion (#16694).

PR Structure

  • (aa12df7) First commit is pure re-implementation without any fix which fails compilation
  • (cd41822) Second commit is fix for rosetta
  • (d3ece53) Third commit is fix for archive
  • (6abe968) updating toolchains
  • (2803494) make fail hard all perf tests which were previously failing due to memory leak issue

Previous PR was reverted as it introduced regression in rosetta. Lesson was learnt and we added more E2E tests for rosetta which now will catch it (#16692). What is more, we stabilized tests here (#16695).

Regarding source of the issue:

After caqti update postgres postgres lost information about type of parameter in some cases. Especially in where clause in search command:

  ... WHERE ($1 IS NULL OR (b.height <= $1)) AND (($2 IS NULL OR (u.hash = $2)) ....

Which resulted in postgres error

could not determine data type of parameter IS NULL

Solution

The possible solution is to always add ::{type} suffix to parameter when using it. For example $1::int or $2::text. For this purpose is modified a bit code responsible for formatting parameter and always use ::{type} suffix for checking for null and CAST when comparing with expected value

How it is tested:

Currently All known rosetta tests are automated in CI:

We added load test to measure memory gain:

We added new test coverage as well for archive node:

Performance improvements metrics

Archive bootstrap test is used as a load test since it try to insert mainnet ledger to archive db in one go.

Rosetta load test fires steady load and measure postgres memory

Running load tests on orchestrator env using rosetta & archive is now possible, thanks to caqti upgrade. Before archive couldn't bootstrap with such big genesis ledger

Test Before fix After fix Comment
Rosetta Load test PostgreSQL Usage : 3323.45 MiB Test Log PostgreSQL Usage: 2074.36 MiB Test Log Single node tests (rosetta, daemon, archive ) Average TPS: 3.08, 📊 Total Requests: 1850
Bootstrap test PostgreSQL Usage: above 4GiB (assertion failed) Test Log PostgreSQL Usage: 2062 MiB Test Log Single archive node test with bootstrapping from mainnet genesis ledger

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania force-pushed the dkijania/reimplement_caqti_upgrade branch from 5c03c4b to 0dbf742 Compare March 12, 2025 19:56
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania changed the title [WIP] Caqti upgrade Caqti upgrade with fix for rosetta Mar 12, 2025
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-nightly-me

@dkijania dkijania marked this pull request as ready for review March 13, 2025 11:04
@dkijania
Copy link
Member Author

@dkijania
Copy link
Member Author

!ci-toolchain-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-toolchain-me

@dannywillems
Copy link
Member

Is this PR still relevant?

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania force-pushed the dkijania/reimplement_caqti_upgrade branch from 2b3c5fd to f73f6ea Compare June 12, 2025 19:51
@dkijania
Copy link
Member Author

!ci-toolchain-me

@dkijania dkijania force-pushed the dkijania/reimplement_caqti_upgrade branch from f73f6ea to 1494fa9 Compare July 31, 2025 11:09
@dkijania
Copy link
Member Author

!ci-build-me

2 similar comments
@dkijania
Copy link
Member Author

dkijania commented Aug 3, 2025

!ci-build-me

@dkijania
Copy link
Member Author

dkijania commented Aug 4, 2025

!ci-build-me

@dkijania
Copy link
Member Author

dkijania commented Aug 4, 2025

!ci-nightly-me

@dkijania
Copy link
Member Author

dkijania commented Aug 8, 2025

!ci-build-me

@dkijania dkijania force-pushed the dkijania/reimplement_caqti_upgrade branch from e206f8c to 03db4c2 Compare August 12, 2025 12:16
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-nigthly-me

@dkijania dkijania self-assigned this Aug 12, 2025
@dkijania dkijania force-pushed the dkijania/reimplement_caqti_upgrade branch from 9901c66 to f7ec359 Compare August 12, 2025 16:47
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania force-pushed the dkijania/reimplement_caqti_upgrade branch from ba454bc to 2803494 Compare August 13, 2025 10:36
@glyh
Copy link
Member

glyh commented Aug 13, 2025

Although I'm not requested to review this: This is too big, is it possible to break it down to several smaller PRs?

@dkijania
Copy link
Member Author

@glyh I thought about it, but it is a library bump PR so it is hard/artificial to do it partially


echo "Running archive node test"
echo "WARN: Silencing the errors from the archive node test app. One of the tests is expected to fail."
MINA_TEST_POSTGRES_URI=$MINA_TEST_POSTGRES_URI $ARCHIVE_TEST_APP || true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a follow-up PR let's remove the line:

echo "WARN: Silencing the errors from the archive node test app. One of the tests is expected to fail."

@dkijania
Copy link
Member Author

!ci-build-me

Copy link
Member

@SanabriaRusso SanabriaRusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm on infra side. Updated toolchains and updated scripts.

@dkijania dkijania merged commit f56ee20 into compatible Aug 20, 2025
67 checks passed
@dkijania dkijania deleted the dkijania/reimplement_caqti_upgrade branch August 20, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants