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

1588 - remove jsonb from Schema & add config #1700

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Cmdv
Copy link
Contributor

@Cmdv Cmdv commented May 13, 2024

Description

This issue fixes #1588

functionality taken from documentation:

Add Jsonb To Schema

add_jsonb_to_schema

Jsonb data types were removed from the schema to improve inserting performance, but if required, they can be reintroduced by enabling this config.
cardano-db-syncwill throw an error ifadd_jsonb_to_schemawas previously set toenableand then either removed from the configuration file or set todisabled`.

  • Type: string

enum: The value of this property must be equal to one of the following values:

Value Explanation
"enable" Enables adding jsonb data types to the schema.
"disable" Does not add jsonb data types to the schema.

Example

"add_jsonb_to_schema": "enable"

Data Types Effected

When enabling this config, the following tables and columns will be set with the jsonb data type:

Table Column
tx_metadata json
script json
datum value
redeemer_data value
cost_model costs
gov_action_proposal description
off_chain_pool_data json
off_chain_vote_data json

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with fourmolu on version 0.10.1.0 (which can be run with scripts/fourmolize.sh)
  • Self-reviewed the diff

Migrations

  • The pr causes a breaking change of type a,b or c
  • If there is a breaking change, the pr includes a database migration and/or a fix process for old values, so that upgrade is possible
  • Resyncing and running the migrations provided will result in the same database semantically

If there is a breaking change, especially a big one, please add a justification here. Please elaborate
more what the migration achieves, what it cannot achieve or why a migration is not possible.

@Cmdv Cmdv mentioned this pull request May 13, 2024
9 tasks
@sgillespie
Copy link
Contributor

Looks like the tests are failing on macOS. You might want to bump the timeout to ~ 6 seconds

@Cmdv
Copy link
Contributor Author

Cmdv commented May 13, 2024

Looks like the tests are failing on macOS. You might want to bump the timeout to ~ 6 seconds

works on my mac m1 max with 64GB of RAM 😂, will change the timeout :)

Copy link
Contributor

@sgillespie sgillespie left a comment

Choose a reason for hiding this comment

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

Tests are still failing on MacOS, I still suspect it has to do with the slow runners provided by GitHub. I also see some commented-out code. Otherwise, LGTM!

@Cmdv Cmdv force-pushed the 1588-remove-jsonb-from-db branch 2 times, most recently from 26048f6 to 8eda705 Compare May 13, 2024 18:51
sgillespie
sgillespie previously approved these changes May 13, 2024
@kderme
Copy link
Contributor

kderme commented May 21, 2024

Some questions to better understand the pr:

  • How long does it take to run the migrations (from and to jsonb) in a fully populated db
  • Is there any difference in terms of Unicode characters between jsonb and text?
  • If there is no difference, Is there any reason we enable only the migration towards one way? I believe the initial motivation was solving the unaccepted unicode errors, but it may not apply.
  • Do we want to allow users to manually change from and to text-jsonb? Could that cause issues?

@@ -92,6 +93,14 @@ metadataValueToJsonNoSchema = conv
. Aeson.Text.encodeToLazyText
$ conv v

txMetadataValueToText :: TxMetadataValue -> Text
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed?

@Cmdv
Copy link
Contributor Author

Cmdv commented May 21, 2024

@kderme

Some questions to better understand the pr:

  • How long does it take to run the migrations (from and to jsonb) in a fully populated db

Going to test that out today

  • Is there any difference in terms of Unicode characters between jsonb and text?

The unicode automatically get converted by postgresql when changing the data type from jsonb -> VARCHAR

  • If there is no difference, Is there any reason we enable only the migration towards one way? I believe the initial motivation was solving the unaccepted unicode errors, but it may not apply.

I thought the reasoning behind this was to improve speeds as jsonb is slower to insert?
It's current state is for the user to only be able to put back the jsonb type after it is removed in migration file.

  • Do we want to allow users to manually change from and to text-jsonb? Could that cause issues?

The problem being if a user wants to have jsonb they can put it back. But once they have done that, there is no going back. Actually the only other way would be for them to manually run the migrations again to set it to VARCHAR and that could be documented. Just seemed handy to be able to do that from the config it self.

@kderme
Copy link
Contributor

kderme commented May 21, 2024

Some questions to better understand the pr:

  • Is there any difference in terms of Unicode characters between jsonb and text?

The unicode automatically get converted by postgresql when changing the data type from jsonb -> VARCHAR

To elaborate on my question. For jsonb type, we need to run checks, like containsUnicodeNul, which results in some values not being inserted eventually. Does the text suffer from the same issue?

  • If there is no difference, Is there any reason we enable only the migration towards one way? I believe the initial motivation was solving the unaccepted unicode errors, but it may not apply.

I thought the reasoning behind this was to improve speeds as jsonb is slower to insert? It's current state is for the user to only be able to put back the jsonb type after it is removed in migration file.

Yes the motivation of changing jsonb is syncing speed. However the pr only allows a migration towards one direction. What is the motivation for this restriction?

@Cmdv Cmdv force-pushed the 1588-remove-jsonb-from-db branch 5 times, most recently from 3c16a73 to b401102 Compare May 21, 2024 18:12
@Cmdv Cmdv force-pushed the 1588-remove-jsonb-from-db branch from b401102 to 568f097 Compare June 5, 2024 10:47
@Cmdv
Copy link
Contributor Author

Cmdv commented Jun 7, 2024

Adding and removing jsonb datatype from DB was tested on full database on mainnet. Each transaction took around 5 minutes, which is was reasonable.

@Cmdv Cmdv force-pushed the 1588-remove-jsonb-from-db branch from d2ce4a0 to 758d311 Compare June 10, 2024 09:15
@Cmdv Cmdv requested a review from kderme June 18, 2024 14:30
Copy link
Contributor

@sgillespie sgillespie left a comment

Choose a reason for hiding this comment

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

LGTM!

@kderme
Copy link
Contributor

kderme commented Jun 19, 2024

@Cmdv did you figure this out?

To elaborate on my question. For jsonb type, we need to run checks, like containsUnicodeNul,
which results in some values not being inserted eventually. Does the text suffer from the same issue?

Since it's not clear if this fixes the dropped metadata issue, let's not change the default, ie enable jsonb by default. Users will be able to disable them by adding the config. This looks good otherwise.

@Cmdv
Copy link
Contributor Author

Cmdv commented Jun 20, 2024

@kderme

Since it's not clear if this fixes the dropped metadata issue, let's not change the default, ie enable jsonb by default. Users will be able to disable them by adding the config. This looks good otherwise.

So you're saying remove the migrations files and have it default to the db having jsonb and if the user doesn't want it then they need to use the config?

@kderme
Copy link
Contributor

kderme commented Jun 20, 2024

So you're saying remove the migrations files and have it default to the db having jsonb and if the user doesn't want it then they need to use the config?

Yes, I assume it won't be too big of a change?

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.

Replace jsonb by text
3 participants