Re: DROP DATABASE is interruptible

Hi,

On 2023-07-07 14:09:08 +0200, Daniel Gustafsson wrote:
> > On 25 Jun 2023, at 19:03, Andres Freund <andres(at)anarazel(dot)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.

Thanks!

> >> 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?

Oops, I stashed them alongside some unrelated changes... Included this time.

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

Indeed.

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

I'd normally be quite careful, people do script psql.

While breaking things when encountering an invalid database doesn't actually
sound like a bad thing, I don't think it fits into any of the existing column
output by psql for \l.

> 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/.

Fixed.

> 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?

I went back and forth about this one. We don't use defines for such things in
all the frontend code today, so the majority of places won't be improved by
adding this. I added them now, which required touching a few otherwise
untouched places, but not too bad.

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

I get confused about this every time. It's not helped by this example in
sources.sgml:

<programlisting>
Primary: could not create shared memory segment: %m
Detail: Failed syscall was shmget(key=%d, size=%u, 0%o).
Hint: the addendum
</programlisting>

Which notably does not use punctuation for the hint. But indeed, later we say:
<para>
Detail and hint messages: Use complete sentences, and end each with
a period. Capitalize the first word of sentences. Put two spaces after
the period if another sentence follows (for English text; might be
inappropriate in other languages).
</para>

> + 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.

Yep.

> + 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.)

I don't really have an opinion on it. Added.

elog(DEBUG2,
"skipping invalid database \"%s\" while computing relfrozenxid",
NameStr(dbform->datname));
and
elog(DEBUG2,
"autovacuum: skipping invalid database \"%s\"",
NameStr(pgdatabase->datname));

Updated patches attached.

Not looking forward to fixing all the conflicts.

Does anybody have an opinion about whether we should add a dedicated field to
pg_database for representing invalid databases in HEAD? I'm inclined to think
that it's not really worth the cross-version complexity at this point, and
it's not that bad a misuse to use pg_database.datconnlimit.

Greetings,

Andres Freund