Date: Mon, 28 Nov 2022 15:40:39 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
	Simon Riggs <simon.riggs@enterprisedb.com>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Maxim Orlov <orlovmg@gmail.com>, Amul Sul <sulamul@gmail.com>,
	Bruce Momjian <bruce@momjian.us>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: O(n) tasks cause lengthy startups and checkpoints
Message-ID: <20221128234039.GA1119654@nathanxps13>
References: <20220824164624.GA1302727@nathanxps13>
 <20220902220744.GA1261322@nathanxps13>
 <20220923174154.GA1111614@nathanxps13>
 <20221106223842.GA620653@nathanxps13>
 <20221124001907.GA576983@nathanxps13>
 <CANbhV-EagKLoUH7tLEfg__VcLu37LY78F8gvLMzHrRZyZKm6sw@mail.gmail.com>
 <20221127233434.GA878043@nathanxps13>
 <CANbhV-GXQ+owyk_jnowFRJEVX1TDgQrnQrgQb6C87V6jTecpZA@mail.gmail.com>
 <20221128183129.ip3g5vrcbjrngpp2@awork3.anarazel.de>
 <CA+TgmoY55=uY=_vD-bQX4XjA0BWRC0JfVHTG=jPedXaPc+v4Mg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="HcAYCG3uE/tztfnV"
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoY55=uY=_vD-bQX4XjA0BWRC0JfVHTG=jPedXaPc+v4Mg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221128234039.GA1119654%40nathanxps13>
Precedence: bulk


--HcAYCG3uE/tztfnV
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Okay, here is a new patch set.  0004 adds logic to prevent custodian tasks
from delaying shutdown.

I haven't added any logging for long-running tasks yet.  Tasks might
ordinarily take a while, so such logs wouldn't necessarily indicate
something is wrong.  Perhaps we could add a GUC for the amount of time to
wait before logging.  This feature would be off by default.  Another option
could be to create a log_custodian GUC that causes tasks to be logged when
completed, similar to log_checkpoints.  Thoughts?

On Mon, Nov 28, 2022 at 01:37:01PM -0500, Robert Haas wrote:
> On Mon, Nov 28, 2022 at 1:31 PM Andres Freund <andres@anarazel.de> wrote:
>> On 2022-11-28 13:08:57 +0000, Simon Riggs wrote:
>> > On Sun, 27 Nov 2022 at 23:34, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> > > > Rather than explicitly use DEBUG1 everywhere I would have an
>> > > > #define CUSTODIAN_LOG_LEVEL     LOG
>> > > > so we can run with it in LOG mode and then set it to DEBUG1 with a one
>> > > > line change in a later phase of Beta
>> > >
>> > > I can create a separate patch for this, but I don't think I've ever seen
>> > > this sort of thing before.
>> >
>> > Much of recovery is coded that way, for the same reason.
>>
>> I think that's not a good thing to copy without a lot more justification than
>> "some old code also does it that way". It's sometimes justified, but also
>> makes code harder to read (one doesn't know what it does without looking up
>> the #define, line length).
> 
> Yeah. If people need some of the log messages at a higher level during
> development, they can patch their own copies.
> 
> I think there might be some argument for having a facility that lets
> you pick subsystems or even individual messages that you want to trace
> and pump up the log level for just those call sites. But I don't know
> exactly what that would look like, and I don't think inventing one-off
> mechanisms for particular cases is a good idea.

Given this discussion, I haven't made any changes to the logging in the new
patch set.

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

--HcAYCG3uE/tztfnV
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v15-0001-Introduce-custodian.patch"