Skip to content

Postgresql-setup script updates #1935

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

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

rdlrt
Copy link
Contributor

@rdlrt rdlrt commented Jan 22, 2025

Description

The postgresql-setup script (as well as docker restore functionality) seems to be only used/tested in a standard configuration and old version of postgresql. With the new options available to dbsync configuration, the setup fails over. This PR hopes to add a few minor updates that are hopefully easy to introduce as non-breaking changes and would largely assist us sticking to official images and not have to maintain a fork.

Additionally , there is a bump of references to postgresql 14 to 17 as pg14 is ancient and misses out on lot of value additions (eg: being able to add better compression on tx_cbor, parallel index maintainance without locks)

  • docker-compose*.yml: Bump postgresql version to 17, node version to 10.1.4 and dbsync version to 13.6.0.4
  • postgresql-setup.sh:
    • Parse psql output using csv instead of manipulating white chars or table seperators
    • Use strict checks for ${PGDATABASE} as there can be another database with a prefix/suffix attached, giving back false outputs
    • When creating/restoring a snapshot, existence of ledger_file should be optional, as there are dbsync config options that don't require lstate files
    • When restoring a snapshot (eg: across postgres versions or for external systems that may have views/stored functions on database being restored), there will be cases where an informational error may be expected. The presence of --exit-on-error flag prevents use of these snapshots, especially when run from docker created by nix-ops (where it isnt as straightforward to override scripts)

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.

@rdlrt rdlrt requested review from a team as code owners January 22, 2025 23:24
@erikd
Copy link
Contributor

erikd commented Jan 22, 2025

@sgillespie We might need your help on the Nix stuff.

@sgillespie
Copy link
Contributor

@sgillespie We might need your help on the Nix stuff.

I think we just need to update nixpkgs/haskell.nix

@sgillespie
Copy link
Contributor

sgillespie commented Jan 24, 2025

Unfortunately, we will not be able to use postgresql_17 until we update the nixpkgs pin for haskell.nix, currently in progress: input-output-hk/haskell.nix#2307

@rdlrt
Copy link
Contributor Author

rdlrt commented Jan 28, 2025

Will re-base and amend once #1936 is merged

@sgillespie
Copy link
Contributor

Will re-base and amend once #1936 is merged

It's done, you should now be able to rebase from master

@rdlrt rdlrt force-pushed the script-updates branch 2 times, most recently from d094e20 to aa63ef4 Compare January 30, 2025 23:20
@rdlrt
Copy link
Contributor Author

rdlrt commented Jan 31, 2025

It's done, you should now be able to rebase from master

Thanks a lot @sgillespie , updated this PR

Cmdv
Cmdv previously approved these changes Jan 31, 2025
Copy link
Contributor

@Cmdv Cmdv left a comment

Choose a reason for hiding this comment

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

🎉 :shipit: Thanks for the contribution @rdlrt

@rdlrt rdlrt force-pushed the script-updates branch 4 times, most recently from 2d1b4ef to af56012 Compare February 3, 2025 07:04
--no-owner \
--exit-on-error \
--no-owner \
"$( [ -z "${SKIP_RESTORE_ERROR}" ] && echo " --exit-on-error" )" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't quoting this make it add "" to the list of arguments? I suspect this will result in:

pg_restore: error: too many command-line arguments (first is "")

@sgillespie
Copy link
Contributor

Thanks for putting this together @rdlrt . I have one concern about the CLI arguments to pg_restore that I mentioned above, otherwise LGTM. Once you address that I'll add my approval

@rdlrt
Copy link
Contributor Author

rdlrt commented Feb 3, 2025

Thanks @sgillespie - Done

@sgillespie
Copy link
Contributor

Can you also add another shellcheck exception?

In ./scripts/postgresql-setup.sh line 208:
            $( [ -z "${SKIP_RESTORE_ERROR:-}" ] && echo --exit-on-error ) \
            ^-- SC2046 (warning): Quote this to prevent word splitting.

docker-compose*.yml: Bump node version to 10.1.4 and dbsync version to 13.6.0.4
postgresql-setup.sh:
- Parse psql output using csv instead of manipulating white chars or table seperators
- Use strict checks for ${PGDATABASE} as there can be another database with a prefix/suffix attached
- When creating/restoring a snapshot, existence of ledger_file should be optional, as there are dbsync config options that don't require lstate files, accordingly - make presence of ledger-state-file optional
- When restoring a snapshot (eg: across postgres versions), there will be cases where an informational error may be expected. The presence of exit-on-error prevents use of these snapshots, especially when run from docker created by nix-ops (where it isnt as straightforward to override scripts)
@rdlrt
Copy link
Contributor Author

rdlrt commented Feb 4, 2025

Can you also add another shellcheck exception?

In ./scripts/postgresql-setup.sh line 208:
            $( [ -z "${SKIP_RESTORE_ERROR:-}" ] && echo --exit-on-error ) \
            ^-- SC2046 (warning): Quote this to prevent word splitting.

That line [albeit surprisingly] does not seem to pop up a warning anymore on latest shellcheck v0.9.0-1, but added an exclusion regardless

@sgillespie
Copy link
Contributor

Can you also add another shellcheck exception?

In ./scripts/postgresql-setup.sh line 208:
            $( [ -z "${SKIP_RESTORE_ERROR:-}" ] && echo --exit-on-error ) \
            ^-- SC2046 (warning): Quote this to prevent word splitting.

That line [albeit surprisingly] does not seem to pop up a warning anymore on latest shellcheck v0.9.0-1, but added an exclusion regardless

Interesting, I'll have to revisit next time we update nixpkgs

@sgillespie sgillespie merged commit 16b4cea into IntersectMBO:master Feb 5, 2025
10 of 11 checks passed
@rdlrt rdlrt deleted the script-updates branch April 1, 2025 09:54
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.

4 participants