Add support for database schema in PostgreSQL #8819

guillep2k · 2019-11-04T04:26:53Z

Currently Gitea does not support using a database schema different from the default "public". With this PR, a schema different from the default can be specified in app.ini and during install.

codecov-io · 2019-11-04T04:50:49Z

Codecov Report

Merging #8819 into master will increase coverage by 0.14%.
The diff coverage is 63.15%.

@@            Coverage Diff            @@
##           master   #8819      +/-   ##
=========================================
+ Coverage   42.25%   42.4%   +0.14%     
=========================================
  Files         605     605              
  Lines       79268   79313      +45     
=========================================
+ Hits        33497   33631     +134     
+ Misses      41637   41522     -115     
- Partials     4134    4160      +26

Impacted Files	Coverage Δ
modules/auth/auth_form.go	`100% <ø> (ø)`	⬆️
modules/auth/ldap/ldap.go	`54.4% <ø> (ø)`	⬆️
models/user.go	`50.16% <0%> (ø)`	⬆️
routers/api/v1/repo/issue_reaction.go	`75.27% <0%> (ø)`	⬆️
routers/api/v1/repo/issue_stopwatch.go	`75.55% <0%> (ø)`	⬆️
routers/repo/issue_dependency.go	`0% <0%> (ø)`	⬆️
routers/repo/issue.go	`37.91% <0%> (-0.12%)`	⬇️
cmd/admin_auth_ldap.go	`79.32% <0%> (ø)`	⬆️
routers/api/v1/repo/issue_comment.go	`56.33% <0%> (ø)`	⬆️
routers/repo/activity.go	`31.5% <0%> (-1.83%)`	⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b16d5af...2cdda3d. Read the comment docs.

lafriks · 2019-11-04T20:02:16Z

Does this also update x.Exec queries?

guillep2k · 2019-11-04T20:17:15Z

Does this also update x.Exec queries?

I've installed Gitea from scratch in a database with no permissions on public for the gitea user and no query failed on me -and there's plenty of those-, but I'll check tonight and document a few cases here.

guillep2k · 2019-11-04T21:55:22Z

I'm sorry, it turns out that certain operations are problematic. Specifically, those escaping the user table get badly escaped:

2019/11/04 18:49:28 .../xorm/session_raw.go:81:queryRows() [I] [SQL] SELECT * FROM "schema5152".""user"" INNER JOIN schema5152.issue_assignees ON assigne
e_id = "user".id WHERE (issue_assignees.issue_id = $1) []interface {}{1} - took: 375.802µs
2019/11/04 18:49:28 ...outers/repo/issue.go:581:NewIssuePost() [E] NewIssue: newIssue: pq: zero-length delimited identifier at or near """"

Other Exec() can be sorted out by running an ALTER USER SET search_path = schema_name, but this particular case is apparently not escaped correctly in xorm.

I should have tested thoroughly. 😞

I'm marking this as WIP.

guillep2k · 2019-11-05T01:22:47Z

Depends on xorm/xorm#1476

lafriks · 2019-11-06T14:22:34Z

imho setting ALTER USER SET search_path defeats purpose of this as primary usecase for this is to be able to use same db for multiple instances/software.

guillep2k · 2019-11-06T19:55:56Z

imho setting ALTER USER SET search_path defeats purpose of this as primary usecase for this is to be able to use same db for multiple instances/software.

@lafriks Mmmm..... I hadn't considered the scenario of many consumers using the the same login. I think Gitea should have its own login (user) and it can share the database with many other logins (users), but with its own schema. Other consumers should have their own logins and credentials.

The bug reported in #5152 talks about the bad practice of having a user's tables in the public schema, and I think that's when a single database is shared among different users. This PR should at least take care of that, and only the gitea user needs the ALTER USER statement.

Anyway, I feel that it should be xorm's job to append the schema name to all tables if so configured, even for Exec() statements. It doesn't, and that's the current limitation we have. As soon as xorm supports that properly, the ALTER USER statement is no longer required as the rest of the job is done.

guillep2k · 2019-11-07T14:49:54Z

imho setting ALTER USER SET search_path defeats purpose of this as primary usecase for this is to be able to use same db for multiple instances/software.

Expanding on this concept, the configuration I think should be used in those cases is:

A single database (e.g. development) for all projects.
Three Gitea instances (for whatever reason):
- Gitea1 with Linux user gitea1, dbuser gitea1, schema giteaschema1.
- Gitea2 with Linux user gitea2, dbuser gitea2, schema giteaschema2.
- Gitea3 with Linux user gitea3, dbuser gitea3, schema giteaschema3.
One Bugzilla instance; Linux user apache, dbuser bugzilla, schema bugzilla.
Other software, you get the idea.

The advantage of this is having all databases backed up at the same point in time, plus better resource sharing. There are disadvantages too, but they don't really matter for this discussion.

guillep2k · 2019-11-12T13:21:05Z

xorm/xorm#1476 has been merged. However, until a new version of xorm (and its companion library lafriks/xormstore) are released, this PR will have to wait. 😴

techknowlogick · 2019-12-18T23:38:32Z

A new release of XORM was made (and our go.mod was updated), now just need for update to xormstore.

guillep2k · 2019-12-19T01:59:27Z

A new release of XORM was made (and our go.mod was updated), now just need for update to xormstore.

Thank you. This PR is not in fact affected by xormstore, so I'm removing the WIP flag.

PR ready for review.

lunny · 2019-12-19T02:06:40Z

@guillep2k How about add a test for different schema from public on integration test?

techknowlogick

Can you add the schema to the templates/admin/config.tmpl page (if db type is postgres)?

guillep2k · 2019-12-19T02:58:14Z

Can you add the schema to the templates/admin/config.tmpl page (if db type is postgres)?

@techknowlogick done.

techknowlogick · 2019-12-19T02:58:52Z

Thanks :) Dismissing my blocking review.

change applied.

lunny

I still think we need some tests.

techknowlogick · 2019-12-20T02:43:29Z

I still think we need some tests.

In that case I’ve moved this to 1.12

guillep2k · 2019-12-20T03:56:30Z

I still think we need some tests.

OK, I'll try when I have some time. But for the test to be meaningful we should run the entire integration tests through a different schema (to account for future bugs). So, the options are:

Change the schema Gitea uses for the PostgreSQL tests (and never use public for that).
Make two different tests: public and alternative, and run them both.

I think (1) is preferable, because it will run only once and we can consider it to be thorough. If any bug slips through (e.g. some case was not accounted for), the public schema will simply not be available and the test will fail. (2) means adding more load to our test servers and I don't see a real advantage to it.

lunny · 2019-12-20T04:47:13Z

@guillep2k I'm OK to just use 1) and set SCHEMA as a non-public one.

guillep2k · 2020-01-07T00:02:29Z

It turns out that supporting the migration process requires additional changes in xorm. Currently the tests passes if the database is created from scratch, but on migration the Sync2() function does some weird juggling with the table names to build the names for the indexes (it's sometimes including the schema name in the index name).

To solve this I've written two more PRs for xorm: xorm/xorm#1505 and xorm/core#65.

We need to wait until those are approved or another solution comes up. 😅

lunny · 2020-01-20T02:46:00Z

both xorm/xorm#1505 and xorm/core#65 merged.

guillep2k · 2020-01-20T03:02:47Z

both xorm/xorm#1505 and xorm/core#65 merged.

OK, I'll update go.mod targeting the latest commit of xorm so we can see how it goes in our CI tests. I have no preference as whether we use a commit or a named version for go.mod, but if a named version is preferred, I can update go.mod again when 0.8.2 is tagged.

guillep2k · 2020-01-20T04:32:20Z

@lunny Done. Build is passing. Now we have to decide if we want to use a commit ref or a version tag for xorm in go.mod.

A note on the tests:

Integration and migration tests use the database dumps from integrations\migration-test. Since I've forced the PostgreSQL tests to use the schema gtestschema instead of the default public, I had to hand-edit all the PostgreSQL dumps and change statements like:

DROP INDEX IF EXISTS public."UQE_watch_watch";

CREATE TABLE public.collaboration (
    id bigint NOT NULL,
    repo_id bigint NOT NULL,
    user_id bigint NOT NULL,
    mode integer DEFAULT 2 NOT NULL
);

CREATE SEQUENCE public.access_id_seq
    START WITH 1
    INCREMENT BY 1
    NO MINVALUE
    NO MAXVALUE
    CACHE 1;

into

DROP INDEX IF EXISTS gtestschema."UQE_watch_watch";

CREATE TABLE gtestschema.collaboration (
    id bigint NOT NULL,
    repo_id bigint NOT NULL,
    user_id bigint NOT NULL,
    mode integer DEFAULT 2 NOT NULL
);

CREATE SEQUENCE gtestschema.access_id_seq
    START WITH 1
    INCREMENT BY 1
    NO MINVALUE
    NO MAXVALUE
    CACHE 1;

(notice the change from public to gtestschema)

This is unavoidable since the tests now expect the data in the gtestschema schema.

zeripath · 2020-01-20T07:26:01Z

@guillep2k that seems to imply that the fixtures loading isn't going in to the giteatest schema?

guillep2k · 2020-01-20T08:18:55Z

@guillep2k that seems to imply that the fixtures loading isn't going in to the giteatest schema?

@zeripath They are, as always. Those are for the unit tests. The db dumps I've changed are used as starting points for the migration and integration tests (I don't know what role the fixtures play in that case, but I changed nothing about them). My only change was using an alternte schema in the case of PGSQL.

zeripath · 2020-01-20T08:26:05Z

They're also used for the integration tests. The migration data should really be dropped after its tests

guillep2k · 2020-01-20T08:35:13Z

They're also used for the integration tests. The migration data should really be dropped after its tests

I didn't change any behaviour there. I just expanded the gzip files, edited the SQL, changed public into gtestschema as appropriate, and gzipped again. I just mentioned this because the commit has them as binary, so a line-by-line review is not possible with those files.

The yaml fixtures are not affected by this PR in any way, since those are loaded through the database connection so they inherit the proper enviroment.

guillep2k · 2020-01-20T08:37:29Z

I meant these files:

zeripath · 2020-01-20T08:45:14Z

Cool. I just misunderstood what you were saying.

Add support for database schema

d07281d

guillep2k mentioned this pull request Nov 4, 2019

gitea does not start - can not find public schema #5152

Closed

2 tasks

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 4, 2019

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 4, 2019

lunny added this to the 1.11.0 milestone Nov 4, 2019

zeripath approved these changes Nov 4, 2019

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 4, 2019

guillep2k changed the title ~~Add support for database schema in PostgreSQL~~ WIP: Add support for database schema in PostgreSQL Nov 4, 2019

Require setting search_path for the db user

5e2dd6d

Merge branch 'master' into fix-5152

0169953

Merge branch 'master' into fix-5152

b3e4c98

guillep2k changed the title ~~WIP: Add support for database schema in PostgreSQL~~ Add support for database schema in PostgreSQL Dec 19, 2019

techknowlogick previously requested changes Dec 19, 2019

View reviewed changes

Add schema setting to admin/config.tmpl

b7f7816

lunny requested changes Dec 20, 2019

View reviewed changes

techknowlogick modified the milestones: 1.11.0, 1.12.0 Dec 20, 2019

guillep2k added 3 commits January 6, 2020 11:58

Merge branch 'master' into fix-5152

45bfbb1

Use a schema different from default for psql tests

c3d051d

Update postgres scripts to use custom schema

e8c9559

Merge branch 'master' into fix-5152

aed2ded

guillep2k added 4 commits January 20, 2020 00:12

Merge branch 'master' into fix-5152

75f1320

Update to xorm/core 0.7.3 and xorm/xorm c37aff9b3a

16c58b1

Merge branch 'fix-5152' of github.com:guillep2k/gitea into fix-5152

4cba8fd

Fix migration test

b16d5af

zeripath approved these changes Jan 20, 2020

View reviewed changes

lunny approved these changes Jan 20, 2020

View reviewed changes

Merge branch 'master' into fix-5152

2cdda3d

sapk merged commit ad1b6d4 into go-gitea:master Jan 20, 2020

guillep2k deleted the fix-5152 branch January 22, 2020 01:15

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for database schema in PostgreSQL #8819

Add support for database schema in PostgreSQL #8819

guillep2k commented Nov 4, 2019

codecov-io commented Nov 4, 2019 •

edited

Loading

lafriks commented Nov 4, 2019

guillep2k commented Nov 4, 2019

guillep2k commented Nov 4, 2019

guillep2k commented Nov 5, 2019

lafriks commented Nov 6, 2019

guillep2k commented Nov 6, 2019

guillep2k commented Nov 7, 2019

guillep2k commented Nov 12, 2019

techknowlogick commented Dec 18, 2019

guillep2k commented Dec 19, 2019

lunny commented Dec 19, 2019

techknowlogick left a comment

guillep2k commented Dec 19, 2019

techknowlogick commented Dec 19, 2019

lunny left a comment

techknowlogick commented Dec 20, 2019

guillep2k commented Dec 20, 2019

lunny commented Dec 20, 2019

guillep2k commented Jan 7, 2020

lunny commented Jan 20, 2020

guillep2k commented Jan 20, 2020

guillep2k commented Jan 20, 2020

zeripath commented Jan 20, 2020

guillep2k commented Jan 20, 2020

zeripath commented Jan 20, 2020

guillep2k commented Jan 20, 2020

guillep2k commented Jan 20, 2020

zeripath commented Jan 20, 2020

Add support for database schema in PostgreSQL #8819

Add support for database schema in PostgreSQL #8819

Conversation

guillep2k commented Nov 4, 2019

codecov-io commented Nov 4, 2019 • edited Loading

Codecov Report

lafriks commented Nov 4, 2019

guillep2k commented Nov 4, 2019

guillep2k commented Nov 4, 2019

guillep2k commented Nov 5, 2019

lafriks commented Nov 6, 2019

guillep2k commented Nov 6, 2019

guillep2k commented Nov 7, 2019

guillep2k commented Nov 12, 2019

techknowlogick commented Dec 18, 2019

guillep2k commented Dec 19, 2019

lunny commented Dec 19, 2019

techknowlogick left a comment

Choose a reason for hiding this comment

guillep2k commented Dec 19, 2019

techknowlogick commented Dec 19, 2019

lunny left a comment

Choose a reason for hiding this comment

techknowlogick commented Dec 20, 2019

guillep2k commented Dec 20, 2019

lunny commented Dec 20, 2019

guillep2k commented Jan 7, 2020

lunny commented Jan 20, 2020

guillep2k commented Jan 20, 2020

guillep2k commented Jan 20, 2020

zeripath commented Jan 20, 2020

guillep2k commented Jan 20, 2020

zeripath commented Jan 20, 2020

guillep2k commented Jan 20, 2020

guillep2k commented Jan 20, 2020

zeripath commented Jan 20, 2020

codecov-io commented Nov 4, 2019 •

edited

Loading