> On 12 Jul 2023, at 03:59, Andres Freund <andres@anarazel.de> wrote:
> On 2023-07-07 14:09:08 +0200, Daniel Gustafsson wrote:
>>> On 25 Jun 2023, at 19:03, Andres Freund <andres@anarazel.de> wrote:
>>> On 2023-06-21 12:02:04 -0700, Andres Freund wrote:
>>> 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.
Looking more at this I wonder if we in HEAD should make this a bit nicer by
extending the --check phase to catch this? I did a quick hack along these
lines in the 0003 commit attached here (0001 and 0002 are your unchanged
patches, just added for consistency and to be CFBot compatible). If done it
could be a separate commit to make the 0002 patch backport cleaner of course.
>>>> 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.
Agreed, it doesn't, it would have to be a new column.
>> + 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>
That's not a very helpful example, and one which may give the wrong impression
unless the entire page is read. I've raised this with a small diff to improve
it on -docs.
> Updated patches attached.
This version of the patchset LGTM.
> 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.
FWIW I think we should use pg_database.datconnlimit for this, it doesn't seem
like a common enough problem to warrant the added complexity and cost.
--
Daniel Gustafsson