Date: Thu, 31 Aug 2023 19:56:13 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>, Jacob Speidel <jacob@extrahop.com>,
	Will Mortensen <will@extrahop.com>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17973: Reinit of pgstats entry for dropped DB can break
 autovacuum daemon
Message-ID: <20230901025613.GB3097511@nathanxps13>
References: <17973-bca1f7d5c14f601e@postgresql.org>
 <20230614213724.gweo666oxqrugm44@awork3.anarazel.de>
 <20230614215744.ddeuii7jcojbp3yv@awork3.anarazel.de>
 <20230614223012.cfyzxyfnt3ggqkt4@awork3.anarazel.de>
 <CAMpnoC5=njD8tLG9q8XQZKi=1SfLehjVPmLJC+pktzGdfZEgoA@mail.gmail.com>
 <CA+pD2MKw6FwU21efw4+kAziFXWB+cnfxiE1824COmp=0x+jwBQ@mail.gmail.com>
 <ZNbRByjoQlUtTkTb@paquier.xyz>
 <20230812004812.4yev47iili57u4hc@awork3.anarazel.de>
 <ZNlxRmI15e1BtCR6@paquier.xyz>
 <ZPE5qXLuMs+/vx9H@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZPE5qXLuMs+/vx9H@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20230901025613.GB3097511%40nathanxps13>
Precedence: bulk

On Fri, Sep 01, 2023 at 10:08:57AM +0900, Michael Paquier wrote:
> On Mon, Aug 14, 2023 at 09:11:50AM +0900, Michael Paquier wrote:
>> At the end, after looking again at the patch and the thread, I'm OK
>> with pgstat_update_dbstats() doing nothing if we don't have
>> MyDatabaseId, because there is nothing to do in this case based on the
>> current structure of the database-level stats, but the expectations
>> surrounding pgstat_update_dbstats() need some clarifications.  Take it
>> as a "I'm OK with the logic of the patch, but it can be improved" ;)
> 
> I have spent quite a few eye hours on the patch, and I would like to
> apply the attached down to v15 at the beginning of next week, likely
> Monday, to take care of the autovacuum scheduler issue.

I've spent much less time staring at the patch than you, but it looks
logically sound to me.  IMO the changes to InitPostgres() are arguably
worthwhile on their own.

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

One other thing that comes to mind is whether this patch creates the
opposite problem for some other code, i.e., it depends on MyDatabaseId
being set before the lock is taken.  But that seems pretty unlikely, and
I'm not aware of anything like that off the top of my head.

Also, IIUC we could simply move the recheck to just after the lock
aquisition and then check MyProc->databaseId in pgstat_update_dbstats(),
but as I mentioned above, the changes to InitPostgres() are kind of nice
independently.  Plus, it's nice to be able to depend on MyDatabaseId in the
same way.

In short, LGTM.

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