Re: DROP DATABASE is interruptible

From: Andres Freund <andres(at)anarazel(dot)de>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org, Evgeny Morozov <postgresql3(at)realityexists(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: DROP DATABASE is interruptible
Date: 2023-06-21 19:02:04
Message-ID: 20230621190204.nsaelabojxppiuix@awork3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

I'm hacking on this bugfix again, thanks to Evgeny's reminder on the other
thread [1].

I've been adding checks for partiall-dropped databases to the following places
so far:
- vac_truncate_clog(), as autovacuum can't process it anymore. Otherwise a
partially dropped database could easily lead to shutdown-due-to-wraparound.
- get_database_list() - so autovacuum workers don't error out when connecting
- template database used by CREATE DATABASE
- pg_dumpall, so we don't try to connect to the database
- vacuumdb, clusterdb, reindexdb, same

It's somewhat annoying that there is no shared place for the relevant query
for the client-side cases.

I haven't yet added checks to pg_upgrade, even though that's clearly
needed. I'm waffling a bit between erroring out and just ignoring the
database? pg_upgrade already fails when datallowconn is set "wrongly", see
check_proper_datallowconn(). Any opinions?

I'm not sure what should be done for psql. It's probably not a good idea to
change tab completion, that'd just make it appear the database is gone. But \l
could probably show dropped databases more prominently?

We don't really have a good place to for database specific
code. dbcommands.[ch] are for commands (duh), but already contain a bunch of
functions that don't really belong there. Seems we should add a
catalog/pg_database.c or catalog/database.c (tbh, I don't really know which we
use for what). But that's probably for HEAD only.

dbcommands.c's get_db_info() seems to have gone completely off the deep
end. It returns information in 14 separate out parameters, and the variables
for that need to all exhaustively be declared. And of course that differs
heavily between releases, making it a pain to backpatch any change. ISTM we
should just define a struct for the parameters - alternatively we could just
return a copy of the pg_database tuple, but it looks like the variable-width
attributes would make that *just* about a loss.

I guess that's once more something better dealt with on HEAD, but damn, I'm
not relishing having to deal with backpatching anything touching it - I think
it might be reasonable to just open-code fetching datconnlimit :/.

This patch is starting to be a bit big, particularly once adding tests for all
the checks mentioned above - but I haven't heard of or thought of a better
proposal :(.

Greetings,

Andres Freund

[1] https://postgr.es/m/01020188d31d0a86-16af92c0-4466-4cb6-a2e8-0e5898aab800-000000%40eu-west-1.amazonses.com

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Drouvot, Bertrand 2023-06-21 19:02:49 Re: ProcessStartupPacket(): database_name and user_name truncation
Previous Message James Coleman 2023-06-21 18:54:13 Re: Memory leak in incremental sort re-scan