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

Add Postgres support #3748

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

Conversation

harryzcy
Copy link
Contributor

@harryzcy harryzcy commented Sep 15, 2023

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #959

Type of change

Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

@harryzcy harryzcy changed the title [2.x] Support postgres [2.0.0] Add Postgres support Sep 15, 2023
server/database.js Outdated Show resolved Hide resolved
server/database.js Outdated Show resolved Hide resolved
db/knex_init_db.js Outdated Show resolved Hide resolved
@louislam louislam changed the title [2.0.0] Add Postgres support Add Postgres support Sep 22, 2023
@JustRedTTG

This comment was marked as off-topic.

@manelpb

This comment was marked as spam.

@CommanderStorm

This comment was marked as resolved.

@chakflying chakflying mentioned this pull request Dec 2, 2023
11 tasks
@gardleopard
Copy link

Hi, I've done a bit to test the postgres support myselves. I found that the user table needs to be renamed as it reserved in postgres. https://www.postgresql.org/docs/current/sql-keywords-appendix.html

Quite a few of the queries needs to be changed as well, like this:

-    let jwtSecretBean = await R.findOne("setting", " `key` = ? ", [
-        "jwtSecret",
-    ]);
+    let jwtSecretBean = await R.knex.select().from("setting").where('key', 'jwtSecret').first();

Im willing to help out, but this seems a bit stale. Any chances of getting postgres support merged if its finished?

@louislam do you think postgres support will be accepted if its completed?

@louislam
Copy link
Owner

It may take a longer to be merged unfortunately, because I have very limited time to test pull requests here. I will likely put my time on other new features first.

And I am honestly not familiar with PostgreSQL, it maybe increases the difficulty for me to maintain this project in the future.

So I would expected this pr will be in v3 or v4. And as MariaDB support is coming, I hope people should try this first.

Anyway, for the user table, maybe it can be escaped:

https://stackoverflow.com/questions/22256124/cannot-create-a-database-table-named-user-in-postgresql

Quite a few of the queries needs to be changed as well, like this:

I prefer not to change the query, because it is hard to review if there are too many changes. What is the error?

@gardleopard
Copy link

It may take a longer to be merged unfortunately, because I have very limited time to test pull requests here. I will likely put my time on other new features first.

And I am honestly not familiar with PostgreSQL, it maybe increases the difficulty for me to maintain this project in the future.

So I would expected this pr will be in v3 or v4. And as MariaDB support is coming, I hope people should try this first.

Anyway, for the user table, maybe it can be escaped:

https://stackoverflow.com/questions/22256124/cannot-create-a-database-table-named-user-in-postgresql

Quite a few of the queries needs to be changed as well, like this:

I prefer not to change the query, because it is hard to review if there are too many changes. What is the error?

That makes a lot of sense. The query fails like this

2023-12-28T21:39:45+01:00 [SERVER] ERROR: Failed to prepare your database: select * from "setting" where  `key` = $1  limit $2 - syntax error at or near "="

I can help out with the mariaDB testing if you want to. How can I help on that?

@louislam
Copy link
Owner

louislam commented Dec 29, 2023

I can help out with the mariaDB testing if you want to. How can I help on that?

@gardleopard Yes, thank you so much, MariaDB should be 70% to 80% ready right now. 2.0.0 Checklist here:
#4171

@Zaid-maker
Copy link
Contributor

Will it be possible to put this PR in v2, as i will give this shot and test it and review the feature. If that possible we can bring this feature in v2

@CommanderStorm
Copy link
Collaborator

See #3748 (comment) #3748 (comment)

=> We currently (v2.0) won't merge this, instead focussing on getting maridb stable first.
=> This will need to wait for another major release.

@brooksvb

This comment was marked as duplicate.

@CommanderStorm CommanderStorm mentioned this pull request Feb 28, 2024
1 task
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label May 19, 2024
@github-actions github-actions bot added pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 20, 2024
@CommanderStorm CommanderStorm added this to the Pending milestone May 28, 2024
@github-actions github-actions bot removed the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label Jun 24, 2024
@dacrudg
Copy link

dacrudg commented Aug 2, 2024

I got so tired of running off of local storage or even longhorn storage which also got corrupted for me at some point, so I just pulled the master branch and merged in this PR today and it works like a charm for MariaDB after a minor fix to database.js::Database::initMariaDB

/* Change the following */
await createTables();
/ * To */
await createTables("mariadb");

PostgreSQL doesn't seem to work out of the box as https://www.npmjs.com/package/redbean-node does not support it yet. So some work needs doing there, but this is awesome! Now it's usable in a "homelab" production environment for MariaDB.

What file did you modify this in? I'd like to get this working, but don't see where. I'm using Docker uptime kuma latest version. Thanks

@Gowtham029
Copy link

Any plan to merge this PR?

@CommanderStorm
Copy link
Collaborator

@Gowtham029
Please refer to my comment above #3748 (comment)

See #3748 (comment) #3748 (comment)

=> We currently (v2.0) won't merge this, instead focussing on getting maridb stable first. > => This will need to wait for another major release.

Incompatabilites like #5087 exist and will need to be resolved one db at a time, otherwise such an endevour is too challenging for the quite limited engineering resources we have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core issues describing changes to the core of uptime kuma area:deployment related to how uptime kuma can be deployed help wanted May need your help to test or answer needs:resolve-merge-conflict pr:needs review this PR needs a review by maintainers or other community members pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgres support