Re: BUG #17973: Reinit of pgstats entry for dropped DB can break autovacuum daemon

On Fri, Sep 01, 2023 at 12:31:09PM +0900, Michael Paquier wrote:
> On Thu, Aug 31, 2023 at 07:56:13PM -0700, Nathan Bossart wrote:
>> On Fri, Sep 01, 2023 at 10:08:57AM +0900, Michael Paquier wrote:
>>> +    /*
>>> +     * Now that we rechecked, we are certain to be connected to a database and
>>> +     * thus can set MyDatabaseId.
>>> +     */
>>> +    MyDatabaseId = dboid;
>> 
>> Perhaps we should expand this comment to make it clear that some code
>> depends on this to avoid races with concurrent database drops/renames.
> 
> Yeah, this could be a bit more detailed, mentioning the dependency
> with the pgstat shutdown callback.  How about appending the following
> sentence in a second paragraph, say:
> "Setting MyDatabaseId after rechecking the existence of the database
> matters for instance with the shutdown callback of the shared
> statistics, as pgstat_report_stat() will update database statistics
> depending on MyDatabaseId."

Something along those lines seems fine to me.  FWIW this is what I had in
my head:

    It is important that MyDatabaseId only be set once we are sure that the
    target database can no longer be concurrently dropped or renamed.  Some
    code depends on this.  For example, without this guarantee,
    pgstat_update_dbstats() could create entries for databases that were
    just dropped, which will lead to problems down the road.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com