> On 25 Jun 2023, at 19:03, Andres Freund <and...@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
