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

Postgres support #5355

Open
wants to merge 87 commits into
base: develop
Choose a base branch
from

Conversation

NodudeWasTaken
Copy link
Contributor

@NodudeWasTaken NodudeWasTaken commented Oct 8, 2024

Hello, this is an attempt at implementing postgres support.
If you want to test this branch, you can add these changes to your config.yml:

database: postgresql://username:password@localhost/dbname

In the ui setup you must set the database path as the connection string to use this.
Also if you want anonymise to work a backup path has to be set

There are alot of changes, highlights:

  • Seperate migrations for postgresql
  • Abstractions around the database class
  • Several SQL changes, particularly no lastinsertid and removal of the distinctids selector
    lastInsertId is a SQLite specific feature, other databases need you to specify "returning id". Which our version of SQLite supports, but goqu doesnt mark it as supported, so we update to a dev version.
    Call regexp as a function, as postgresql doesnt support named operators.
  • Blob changes to make it cross-db compatible
  • Postgres DLL extension to add missing functionality
    Postgresql C/CGo extension to support missing functionality (basename, phash_distance, regexp):
    https://github.com/NodudeWasTaken/stash_pge
  • Migration script
    https://github.com/NodudeWasTaken/stash_sqlite_to_pgsql

Some particularly egregious changes are:

  • Lack of backups for postgresql
    This will be the admins responsibility
  • Lack of automated testing
    We should probably add a postgres test
    PGSQL_TEST='postgresql://username:password@localhost/dbname_for_testing' go test -tags "sqlite_stat4 sqlite_math_functions integration" ./...

Some particularly lackings are:

  • automated testing on git
  • that one bug that causes tests on postgres to sometimes fail
    I added testmode which mitigated it a little, but it still happens sometimes.
  • find the optimal number of concurrent read/writes
  • it would be nice to warn if the dll isnt installed
  • There is also a bug in the funscript speed generator that causes it to sometimes return a max value negative 64 bit integer, and i thought it was ridiculous to use a bigint for a value that's < 800, so i wrote a clamper into the migrator
    But i fear that it may cause insertion issues, maybe, since the thing that updates it doesnt clamp the value, and it unlike sqlite actually returns an error on overflow.
  • Heatmaps disappear sometimes
  • Its slower than sqlite, resulting in many slow sql logs

To look at (help me):

  • Number of concurrent reader/writers
  • Warn if extension missing
  • UI setup for postgresql
  • Testing for pgsql
  • Code cleanup, some of it is a little ugly

You can use dbeaver for testing the database, its nice for browsing various databases like sqlite and postgresql.
To support disable foreign keys we do (if anybody knows a better way, let me know):
"SET session_replication_role = replica;"
which requires extra postgres user permissions, just a note.

This is mainly a request for comments, or help, mainly help finishing this.

Fixes #3892

Comment on lines 49 to 62
if disableForeignKeys {
_, err = conn.Exec("SET session_replication_role = replica;")

if err != nil {
return nil, fmt.Errorf("conn.Exec(): %w", err)
}
}
if !writable {
_, err = conn.Exec("SET default_transaction_read_only = ON;")

if err != nil {
return nil, fmt.Errorf("conn.Exec(): %w", err)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest not supporting "disable foreign key" and "read-only" in Postgres in the application layer.

They exist for SQLite because of limitations in the DB, such as some messiness with foreign keys. For connecting to Postgres as read-only, I'd strongly recommend that admins create a user with read-only permissions to the DB, rather than doing that in Stash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this is ugly and should be removed.
But that would necessitate adding another connection string.
Im gonna have to think about it.

Or simply add "database_string_ro" 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

If you absolutely need to tell Stash to make this RO, you could add a query stirng parameter to the connection string.

Something like ?readonly=1 to the URL... and then we remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im gonna let a reviewer weigh-in on this

Copy link
Contributor

@its-josh4 its-josh4 left a comment

Choose a reason for hiding this comment

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

Some initial review/feedback from me

pkg/sqlite/repository.go Outdated Show resolved Hide resolved
pkg/sqlite/repository.go Outdated Show resolved Hide resolved
pkg/sqlite/repository.go Outdated Show resolved Hide resolved
pkg/sqlite/sql.go Show resolved Hide resolved
pkg/sqlite/scene_filter.go Outdated Show resolved Hide resolved
pkg/sqlite/migrationsPostgres/1_initial.up.sql Outdated Show resolved Hide resolved
pkg/sqlite/migrationsPostgres/1_initial.up.sql Outdated Show resolved Hide resolved
pkg/sqlite/migrationsPostgres/1_initial.up.sql Outdated Show resolved Hide resolved
pkg/sqlite/migrationsPostgres/1_initial.up.sql Outdated Show resolved Hide resolved
internal/manager/init.go Outdated Show resolved Hide resolved
Sort by nulls last fix for tests.
UUID handler
Switch strftime for native postgresql types
Fix bug in performer studio filter
LIKE in SQLITE is case insensetive by default, where its SENSETIVE in postgres.
Postgres has ILIKE which is insensitive, we should use that
Fix findExactDuplicateQuery for sqlite and pgsql use.
Fix random sort for pgsql.
Fix studioquery test by setting the id as a number (sqlite isnt strict, but pgsql is)
Its slightly more performant, and slightly prettier
…in postgresql.

Postgres is stricter and doesnt allow excess args.
So we initialize the specific database type in PostInit, which also makes it unavailable to the manager.
@NodudeWasTaken NodudeWasTaken marked this pull request as ready for review November 9, 2024 13:18
NodudeWasTaken and others added 3 commits November 11, 2024 20:15
Fix a bug where phash distance didnt work.
Fix a bug where i wrongly wrote getDBLike in a string in captionCriterionHandler.
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.

[Feature] Support Postgres as DB backend
3 participants