Re: DROP DATABASE is interruptible

> On 25 Jun 2023, at 19:03, Andres Freund <andres@anarazel.de> wrote:
> On 2023-06-21 12:02:04 -0700, Andres Freund wrote:
>> I'm hacking on this bugfix again, 

This patch LGTM from reading through and testing (manually and with your
supplied tests in the patch), I think this is a sound approach to deal with
this.

>> 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
> 
> Also pg_amcheck.

That seems like an exhaustive list to me, I was unable to think of any other
place which would need the same treatment.  pg_checksums does come to mind but
it can clearly not see the required info so there doesn't seem like theres a
lot to do about that.

>> 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?
> 
> There don't need to be explict checks, because pg_upgrade will fail, because
> it connects to every database. Obviously the error could be nicer, but it
> seems ok for something hopefully very rare. I did add a test ensuring that the
> behaviour is caught.

I don't see any pg_upgrade test in the patch?

> It's somewhat odd that pg_upgrade prints errors on stdout...

There are many odd things about pg_upgrade logging, updating it to use the
common logging framework of other utils would be nice.

>> 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?
> 
> I have not done that. I wonder if this is something that should be done in the
> back branches?

Possibly, I'm not sure where we usually stand on changing the output format of
\ commands in psql in minor revisions.

A few small comments on the patch:

+ * Max connections allowed (-1=no limit, -2=invalid database). A database
+ * is set to invalid partway through eing dropped.  Using datconnlimit=-2
+ * for this purpose isn't particularly clean, but is backpatchable.
Typo: s/eing/being/.  A limit of -1 makes sense, but the meaning of -2 is less
intuitive IMO.  Would it make sense to add a #define with a more descriptive
name to save folks reading this having to grep around and figure out what -2
means?

+	errhint("Use DROP DATABASE to drop invalid databases"));
Should end with a period as a complete sentence?

+	errmsg("cannot alter invalid database \"%s\"", stmt->dbname),
+	errdetail("Use DROP DATABASE to drop invalid databases"));
Shouldn't this be an errhint() instead? Also ending with a period.

+	if (database_is_invalid_form((Form_pg_database) dbform))
+		continue;
Would it make sense to stick a DEBUG2 log entry in there to signal that such a
database exist? (The same would apply for the similar hunk in autovacuum.c.)

--
Daniel Gustafsson