From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Bossart, Nathan <bossartn@amazon.com>
Cc: Maxim Orlov <orlovmg@gmail.com>
Cc: Amul Sul <sulamul@gmail.com>
Cc: Bruce Momjian <bruce@momjian.us>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: O(n) tasks cause lengthy startups and checkpoints
Date: Sun, 2 Apr 2023 12:30:30 -0700
Message-ID: <20230402193030.GA25018@nathanxps13> (raw)
In-Reply-To: <1031491.1680457205@sss.pgh.pa.us>
References: <20221130051833.GB1677223@nathanxps13>
<CALj2ACVod5vJS7M7LqmPXfnhmxRPKB2CQKYBvUK3iqhomzqh1Q@mail.gmail.com>
<CALj2ACUCvhtXO7Ay-7ogxEBAvt74VCajAj=CqWDDTLsZT4+zTA@mail.gmail.com>
<20221201214026.GA1799688@nathanxps13>
<CALj2ACWupmFrhv4ZOdiB8rrt2UuC8bpCO_v=Sbu+fFEoSgYoNw@mail.gmail.com>
<20221202191507.GA2277157@nathanxps13>
<CALj2ACW1F-T866iaqA_9h9fA+Axdyc86CHz7_k55RGH5HH2rtA@mail.gmail.com>
<20230203054808.GA83788@nathanxps13>
<20230217234344.GA3357392@nathanxps13>
<1031491.1680457205@sss.pgh.pa.us>
On Sun, Apr 02, 2023 at 01:40:05PM -0400, Tom Lane wrote:
> I took a brief look through v20, and generally liked what I saw,
> but there are a few things troubling me:
Thanks for taking a look.
> * The comments for CustodianEnqueueTask claim that it won't enqueue an
> already-queued task, but I don't think I believe that, because it stops
> scanning as soon as it finds an empty slot. That data structure seems
> quite oddly designed in any case. Why isn't it simply an array of
> need-to-run-this-one booleans indexed by the CustodianTask enum?
> Fairness of dispatch could be ensured by the same state variable that
> CustodianGetNextTask already uses to track which array element to
> inspect next. While that wouldn't guarantee that tasks A and B are
> dispatched in the same order they were requested in, I'm not sure why
> we should care.
That works. Will update.
> * I don't much like cust_lck, mainly because you didn't bother to
> document what it protects (in general, CustodianShmemStruct deserves
> more than zero commentary). Do we need it at all? If the task-needed
> flags were sig_atomic_t not bool, we probably don't need it for the
> basic job of tracking which tasks remain to be run. I see that some
> of the tasks have possibly-non-atomically-assigned parameters to be
> transmitted, but restricting cust_lck to protect those seems like a
> better idea.
Will do.
> * Not quite convinced about handle_arg_func, mainly because the Datum
> API would be pretty inconvenient for any task with more than one arg.
> Why do we need that at all, rather than saying that callers should
> set up any required parameters separately before invoking
> RequestCustodian?
I had done it this way earlier, but added the Datum argument based on
feedback upthread [0]. It presently has only one proposed use, anyway, so
I think it would be fine to switch it back for now.
> * Why does LookupCustodianFunctions think it needs to search the
> constant array?
The order of the tasks in the array isn't guaranteed to match the order in
the CustodianTask enum.
> * The original proposal included moving RemovePgTempFiles into this
> mechanism, which I thought was probably the most useful bit of the
> whole thing. I'm sad to see that gone, what became of it?
I postponed that based on advice from upthread [1]. I was hoping to start
a dedicated thread for that immediately after the custodian infrastructure
was committed. FWIW I agree that it's the most useful task of what's
proposed thus far.
[0] https://postgr.es/m/20220703172732.wembjsb55xl63vuw%40awork3.anarazel.de
[1] https://postgr.es/m/CANbhV-EagKLoUH7tLEfg__VcLu37LY78F8gvLMzHrRZyZKm6sw%40mail.gmail.com
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, bharath.rupireddyforpostgres@gmail.com, robertmhaas@gmail.com, andres@anarazel.de, bossartn@amazon.com, orlovmg@gmail.com, sulamul@gmail.com, bruce@momjian.us
Subject: Re: O(n) tasks cause lengthy startups and checkpoints
In-Reply-To: <20230402193030.GA25018@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox