Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#29613 closed Bug (fixed)

Allow --keepdb to work on PostgreSQL if the database exists but the user can't create databases

Reported by: Paul McDermott Owned by: Mariusz Felisiak
Component: Testing framework Version: 2.0
Severity: Normal Keywords: postgres, --keepdb
Cc: Mariusz Felisiak Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The popular Web Faction hosting service uses a shared database server. Users can create databases using the web UI or XML RPC calls, but not using the SQL CREATE syntax.

Running tests throws a ProgrammingError, with the message 'permission denied to create database', even if the test database has been previously created manually.

The error code for this error is '42501', which appears to correspond to errorcodes.INSUFFICIENT_PRIVILEGE.

django/db/backends/postgresql/creation.py only handles the error errorcodes.DUPLICATE_DATABASE in _execute_create_test_db(), line 35. Because the error code does not match the program exits with a log message. But it would be fine to proceed with the error code '42501' also, making use of the --keepdb mechanism.

This appears to be a regression, as I did not experience this issue either using postgresql_psycopg2 driver or using Django 1.11

Change History (11)

comment:1 Changed 2 years ago by Paul McDermott

The following modification to _execute_create_test_db() got the --keepdb flag working again for me on Web Faction

def _execute_create_test_db(self, cursor, parameters, keepdb=False):
    try:
        super()._execute_create_test_db(cursor, parameters, keepdb)
    except Exception as e:
        allowed_codes = [errorcodes.DUPLICATE_DATABASE, errorcodes.INSUFFICIENT_PRIVILEGE]
        if getattr(e.__cause__, 'pgcode', '') in allowed_codes:
            # All errors except "database already exists" cancel tests.
            self.log('Got an error creating the test database: %s' % e)
            sys.exit(2)
        elif not keepdb:
            # If the database should be kept, ignore "database already
            # exists".
            raise e

comment:2 Changed 2 years ago by Tim Graham

The change in behavior is due to e776dd2db677d58dcb50aea20d3bb191537df25b (#28116). Isn't the problem with your proposal that it would hide cases where the database doesn't exist and the permissions are insufficient to create it?

comment:3 Changed 2 years ago by Mariusz Felisiak

Cc: Mariusz Felisiak added

Agreed with TIm, if you created database in advance, 42P04 - Duplicate database should be raised. Your proposal will hide useful information for all users without sufficient privileges.

comment:4 Changed 2 years ago by Tim Graham

I can understand that PostgreSQL will error with "insufficient permissions". We could to try to emulate CREATE DATABASE IF NOT EXISTS.

comment:5 in reply to:  2 Changed 2 years ago by Paul McDermott

Replying to Tim Graham:

Isn't the problem with your proposal that it would hide cases where the database doesn't exist and the permissions are insufficient to create it?

That may be the case, but a little bit of manual tweaking is implicit in the use of --keepdb for this use case (that is deploying to a shared database environment where one cannot create databases as a user).

I understand that there are other uses for the --keepdb flag to speed up testing. Maybe there should really be 2 flags with different logic:
--dont_try_creating_database_because_it_wont_work; and
--keep_the_database_around_cos_I_like_the_speed

(exaggerating for effect).

comment:6 Changed 2 years ago by Tim Graham

Component: Database layer (models, ORM)Testing framework
Summary: --keepdb flag for postgresql does not work with some database error codesAllow --keepdb to work on PostgreSQL if the database exists but the user can't create databases
Triage Stage: UnreviewedAccepted

I don't think adding another flag is a nice option.

comment:7 Changed 2 years ago by Mariusz Felisiak

Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:8 Changed 2 years ago by Mariusz Felisiak

Has patch: set

comment:9 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:10 Changed 2 years ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 1a9cbf4:

Fixed #29613 -- Fixed --keepdb on PostgreSQL if the database exists and the user can't create databases.

Regression in e776dd2db677d58dcb50aea20d3bb191537df25b.

Thanks Tim Graham for the review.

comment:11 Changed 2 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In c706091:

[2.1.x] Fixed #29613 -- Fixed --keepdb on PostgreSQL if the database exists and the user can't create databases.

Regression in e776dd2db677d58dcb50aea20d3bb191537df25b.

Thanks Tim Graham for the review.
Backport of 1a9cbf41a130def83a7e384955544d08be0fc148 from master

Note: See TracTickets for help on using tickets.
Back to Top