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

change checkpoint_ns to 255 (temp fix) #40

Merged
merged 3 commits into from
Dec 21, 2024
Merged

change checkpoint_ns to 255 (temp fix) #40

merged 3 commits into from
Dec 21, 2024

Conversation

a180285
Copy link
Contributor

@a180285 a180285 commented Dec 20, 2024

try fix #39

but mysql don't support TEXT as a key. this MR is just a temp fix.
for long term fix. I think we should remove checkpoint_ns from primary key

Copy link
Owner

@tjni tjni left a comment

Choose a reason for hiding this comment

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

Left one question that can be figured out later. Thank you for this contribution!

If you find that 255 is also not enough, we can remove it from the primary key and just add an index on the column.

@@ -119,6 +119,7 @@ def setup(self) -> None:
):
cur.execute(migration)
cur.execute(f"INSERT INTO checkpoint_migrations (v) VALUES ({v})")
cur.execute("COMMIT")
Copy link
Owner

Choose a reason for hiding this comment

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

Do you know why the upstream PostgreSQL checkpointer doesn't need this? (I assumed it was since it expects connections to be constructed using autocommit=True, but I'm open to making this support other cases too if I understand the use case.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my testing, the version value is not up to date. So when I run setup next time. It will try to excute previouse migration. Previous migration will cause error.
I didn't dive much too. But I found execute 'commit' can make version up to date. So I added.
And now setup() can been call multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's caused by autocommit . When I use this repo. I read the readme. Noticed that I need autocommit set to True.

But when I use sqlalchemy import create_engine. I didn't find autocommit setting. After searching, I think the autocommit is set to True by default. So I didn't set autocommit in my code. Just use default value.

@tjni tjni merged commit b54e96c into tjni:main Dec 21, 2024
6 checks passed
@a180285
Copy link
Contributor Author

a180285 commented Dec 21, 2024

@tjni en. 255 is not enough. As I use nested subgraph. And I found that one nested subgraph call will use 40-60 chars for checkpoint-ns. So after about 5 nested subgraph call will cause this issue again.

So temprarly I just avoid too much nested subgraph. But I think about 5 nested subgraph call is not enough for large project

But I'm not testing postgresql, if it also have this limitation or not? As I'm using MySQL in my project now.

@tjni
Copy link
Owner

tjni commented Dec 21, 2024

Let's update the primary key then and see if that fixes things!

@tjni
Copy link
Owner

tjni commented Dec 21, 2024

I pushed migrations to remove checkpoint_ns from the primary keys and increase its max size to 2000 characters. I didn't add an index on it yet; let's see if it matters.

@tjni
Copy link
Owner

tjni commented Dec 21, 2024

I released version 2.0.9 with the removal of checkpoint_ns from the primary keys. Please let me know if there are any issues.

@a180285
Copy link
Contributor Author

a180285 commented Dec 21, 2024

Thank you for the quick reply and fix. It looks work now. Thanks very much.

@tjni
Copy link
Owner

tjni commented Jan 3, 2025

FYI: I released version 2.0.10 which adds back a hash of checkpoint_ns to the primary key. It turns out that this is used in the upsert statements.

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.

Data too long for column 'checkpoint_ns'
2 participants