Subject: Re: O(n) tasks cause lengthy startups and checkpoints

From: Nathan Bossart <[email-masked]>
Date: Thu, 17 Feb 2022 14:58:38 -0800

On Thu, Feb 17, 2022 at 02:28:29PM -0800, Andres Freund wrote:
> As far as I understand, the primary concern are logical decoding serialized
> snapshots, because a lot of them can accumulate if there e.g. is an old unused
> / far behind slot. It should be easy to reduce the number of those snapshots
> by e.g. eliding some redundant ones. Perhaps we could also make backends in
> logical decoding occasionally do a bit of cleanup themselves.
> 
> I've not seen reports of the number of mapping files to be an real issue?

I routinely see all four of these tasks impacting customers, but I'd say
the most common one is the temporary file cleanup.  Besides eliminating
some redundant files and having backends perform some cleanup, what do you
think about skipping the logical decoding cleanup during
end-of-recovery/shutdown checkpoints?  This was something that Bharath
brought up a while back [0].  As I noted in that thread, startup and
shutdown could still take a while if checkpoints are regularly delayed due
to logical decoding cleanup, but that might still help avoid a bit of
downtime.

> The improvements around deleting temporary files and serialized snapshots
> afaict don't require a dedicated process - they're only relevant during
> startup. We could use the approach of renaming the directory out of the way as
> done in this patchset but perform the cleanup in the startup process after
> we're up.

Perhaps this is a good place to start.  As I mentioned above, IME the
temporary file cleanup is the most common problem, so I think even getting
that one fixed would be a huge improvement.

[0] https://postgr.es/m/CALj2ACXkkSL8EBpR7m%3DMt%3DyRGBhevcCs3x4fsp3Bc-D13yyHOg%40mail.gmail.com

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

---

From: Andres Freund <[email-masked]>
Date: Thu, 17 Feb 2022 15:12:47 -0800

Hi,

On 2022-02-17 14:58:38 -0800, Nathan Bossart wrote:
> the most common one is the temporary file cleanup.

I took temp file cleanup and StartupReorderBuffer() "out of consideration" for
custodian, because they're not needed during normal running.

> Besides eliminating some redundant files and having backends perform some
> cleanup, what do you think about skipping the logical decoding cleanup
> during end-of-recovery/shutdown checkpoints?

I strongly disagree with it. Then you might never get the cleanup done, but
keep on operating until you hit corruption issues.

> that one fixed would be a huge improvement.

Cool.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 18 Feb 2022 08:44:54 -0800

>> that one fixed would be a huge improvement.

Hm.  How should this work for standbys?  I can think of the following
options:
	1. Do temporary file cleanup in the postmaster (as it does today).
	2. Pause after allowing connections to clean up temporary files.
	3. Do small amounts of temporary file cleanup whenever there is an
	   opportunity during recovery.
	4. Wait until recovery completes before cleaning up temporary files.

I'm not too thrilled about any of these options.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 18 Feb 2022 12:51:11 -0800

>>> > done in this patchset but perform the cleanup in the startup process after

BTW I know you don't like the dedicated process approach, but one
improvement to that approach could be to shut down the custodian process
when it has nothing to do.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 17 Mar 2022 16:07:58 -0700

It seems unlikely that anything discussed in this thread will be committed
for v15, so I've adjusted the commitfest entry to v16 and moved it to the
next commitfest.

---

From: Simon Riggs <[email-masked]>
Date: Thu, 23 Jun 2022 12:58:15 +0100

> when it has nothing to do.

Having a central cleanup process makes a lot of sense. There is a long
list of potential tasks for such a process. My understanding is that
autovacuum already has an interface for handling additional workload
types, which is how BRIN indexes are handled. Do we really need a new
process? If so, lets do this now.

Nathan's point that certain tasks are blocking fast startup is a good
one and higher availability is a critical end goal. The thought that
we should complete these tasks during checkpoint is a good one, but
checkpoints should NOT be delayed by long running tasks that are
secondary to availability.

Andres' point that it would be better to avoid long running tasks is
good, if that is possible. That can be done better over time. This
point does not block the higher level goal of better availability
asap, so I support Nathan's overall proposals.

-- 
Simon Riggs                http://www.EnterpriseDB.com/

---

From: Robert Haas <[email-masked]>
Date: Thu, 23 Jun 2022 09:46:28 -0400

On Thu, Jun 23, 2022 at 7:58 AM Simon Riggs
<[email-masked]> wrote:
> process?

It seems to me that if there's a long list of possible tasks for such
a process, that's actually a trickier situation than if there were
only one or two, because it may happen that when task X is really
urgent, the process is already busy with task Y.

I don't think that piggybacking more stuff onto autovacuum is a very
good idea for this exact reason. We already know that autovacuum
workers can get so busy that they can't keep up with the need to
vacuum and analyze tables. If we give them more things to do, that
figures to make it worse, at least on busy systems.

I do agree that a general mechanism for getting cleanup tasks done in
the background could be a useful thing to have, but I feel like it's
hard to see exactly how to make it work well. We can't just allow it
to spin up a million new processes, but at the same time, if it can't
guarantee that time-critical tasks get performed relatively quickly,
it's pretty worthless.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Simon Riggs <[email-masked]>
Date: Thu, 23 Jun 2022 18:13:26 +0100

> guarantee that time-critical tasks get performed relatively quickly,

Most of the tasks mentioned aren't time critical.

I have no objection to a new auxiliary process to execute those tasks,
which can be spawned when needed.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 23 Jun 2022 10:15:52 -0700

> guarantee that time-critical tasks get performed relatively quickly,

My intent with this new auxiliary process is to offload tasks that aren't
particularly time-critical.  They are only time-critical in the sense that
1) you might eventually run out of space and 2) you might encounter
wraparound with the logical replication files.  But AFAICT these same risks
exist today in the checkpointer approach, although maybe not to the same
extent.  In any case, 2 seems solvable to me outside of this patch set.

I'm grateful for the discussion in this thread so far, but I'm not seeing a
clear path forward.  I'm glad to see threads like the one to stop doing
end-of-recovery checkpoints [0], but I don't know if it will be possible to
solve all of these availability concerns in a piecemeal fashion.  I remain
open to exploring other suggested approaches beyond creating a new
auxiliary process.

[0] https://postgr.es/m/CA%2BTgmobrM2jvkiccCS9NgFcdjNSgAvk1qcAPx5S6F%2BoJT3D2mQ%40mail.gmail.com

---

From: Simon Riggs <[email-masked]>
Date: Fri, 24 Jun 2022 11:45:22 +0100

On Thu, 23 Jun 2022 at 18:15, Nathan Bossart <[email-masked]> wrote:

> clear path forward.

+1 to add the new auxiliary process.

---

From: Nathan Bossart <[email-masked]>
Date: Sat, 2 Jul 2022 15:05:54 -0700

> +1 to add the new auxiliary process.

I went ahead and put together a new patch set for this in which I've
attempted to address most of the feedback from upthread.  Notably, I've
abandoned 0007 and 0008, added a way for processes to request specific
tasks for the custodian, and removed all the checks for
ShutdownRequestPending.

I haven't addressed the existing transaction ID wraparound risk with the
logical replication files.  My instinct is that this deserveѕ its own
thread, and it might need to be considered a prerequisite to this change
based on the prior discussion here.

---

From: Andres Freund <[email-masked]>
Date: Sat, 2 Jul 2022 15:54:56 -0700

Hi,

On 2022-07-02 15:05:54 -0700, Nathan Bossart wrote:
> +		/* Obtain requested tasks */
> +		SpinLockAcquire(&CustodianShmem->cust_lck);
> +		flags = CustodianShmem->cust_flags;
> +		CustodianShmem->cust_flags = 0;
> +		SpinLockRelease(&CustodianShmem->cust_lck);

Just resetting the flags to 0 is problematic. Consider what happens if there's
two tasks and and the one processed first errors out. You'll loose information
about needing to run the second task.

> +		/* TODO: offloaded tasks go here */

Seems we're going to need some sorting of which tasks are most "urgent" / need
to be processed next if we plan to make this into some generic facility.

> +/*
> + * RequestCustodian
> + *		Called to request a custodian task.
> + */
> +void
> +RequestCustodian(int flags)
> +{
> +	SpinLockAcquire(&CustodianShmem->cust_lck);
> +	CustodianShmem->cust_flags |= flags;
> +	SpinLockRelease(&CustodianShmem->cust_lck);
> +
> +	if (ProcGlobal->custodianLatch)
> +		SetLatch(ProcGlobal->custodianLatch);
> +}

With this representation we can't really implement waiting for a task or
such. And it doesn't seem like a great API for the caller to just specify a
mix of flags.

> +		/* Calculate how long to sleep */
> +		end_time = (pg_time_t) time(NULL);
> +		elapsed_secs = end_time - start_time;
> +		if (elapsed_secs >= CUSTODIAN_TIMEOUT_S)
> +			continue;			/* no sleep for us */
> +		cur_timeout = CUSTODIAN_TIMEOUT_S - elapsed_secs;
> +
> +		(void) WaitLatch(MyLatch,
> +						 WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> +						 cur_timeout * 1000L /* convert to ms */ ,
> +						 WAIT_EVENT_CUSTODIAN_MAIN);
> +	}

I don't think we should have this thing wake up on a regular basis. We're
doing way too much of that already, and I don't think we should add
more. Either we need a list of times when tasks need to be processed and wake
up at that time, or just wake up if somebody requests a task.

> From 5e95666efa31d6c8aa351e430c37ead6e27acb72 Mon Sep 17 00:00:00 2001
> From: Nathan Bossart <[email-masked]>
> Date: Sun, 5 Dec 2021 21:16:44 -0800
> Subject: [PATCH v6 3/6] Split pgsql_tmp cleanup into two stages.
> 
> First, pgsql_tmp directories will be renamed to stage them for
> removal.  Then, all files in pgsql_tmp are removed before removing
> the staged directories themselves.  This change is being made in
> preparation for a follow-up change to offload most temporary file
> cleanup to the new custodian process.

> Note that temporary relation files cannot be cleaned up via the
> aforementioned strategy and will not be offloaded to the custodian.

> ---
>  src/backend/postmaster/postmaster.c |   8 +-
>  src/backend/storage/file/fd.c       | 174 ++++++++++++++++++++++++----
>  src/include/storage/fd.h            |   2 +-
>  3 files changed, 160 insertions(+), 24 deletions(-)
> 
> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
> index e67370012f..82aa0c6307 100644
> --- a/src/backend/postmaster/postmaster.c
> +++ b/src/backend/postmaster/postmaster.c
> @@ -1402,7 +1402,8 @@ PostmasterMain(int argc, char *argv[])
>  	 * Remove old temporary files.  At this point there can be no other
>  	 * Postgres processes running in this directory, so this should be safe.
>  	 */
> -	RemovePgTempFiles();
> +	RemovePgTempFiles(true, true);
> +	RemovePgTempFiles(false, false);

This is imo hard to read and easy to get wrong. Make it multiple functions or
pass named flags in.

> + * StagePgTempDirForRemoval
> + *
> + * This function renames the given directory with a special prefix that
> + * RemoveStagedPgTempDirs() will know to look for.  An integer is appended to
> + * the end of the new directory name in case previously staged pgsql_tmp
> + * directories have not yet been removed.
> + */

It doesn't seem great to need to iterate through a directory that contains
other files, potentially a significant number. How about having a
staged_for_removal/ directory, and then only scanning that?

> +static void
> +StagePgTempDirForRemoval(const char *tmp_dir)
> +{
> +	DIR		   *dir;
> +	char		stage_path[MAXPGPATH * 2];
> +	char		parent_path[MAXPGPATH * 2];
> +	struct stat statbuf;
> +
> +	/*
> +	 * If tmp_dir doesn't exist, there is nothing to stage.
> +	 */
> +	dir = AllocateDir(tmp_dir);
> +	if (dir == NULL)
> +	{
> +		if (errno != ENOENT)
> +			ereport(LOG,
> +					(errcode_for_file_access(),
> +					 errmsg("could not open directory \"%s\": %m", tmp_dir)));
> +		return;
> +	}
> +	FreeDir(dir);
> +
> +	strlcpy(parent_path, tmp_dir, MAXPGPATH * 2);
> +	get_parent_directory(parent_path);
> +
> +	/*
> +	 * get_parent_directory() returns an empty string if the input argument is
> +	 * just a file name (see comments in path.c), so handle that as being the
> +	 * current directory.
> +	 */
> +	if (strlen(parent_path) == 0)
> +		strlcpy(parent_path, ".", MAXPGPATH * 2);
> +
> +	/*
> +	 * Find a name for the stage directory.  We just increment an integer at the
> +	 * end of the name until we find one that doesn't exist.
> +	 */
> +	for (int n = 0; n <= INT_MAX; n++)
> +	{
> +		snprintf(stage_path, sizeof(stage_path), "%s/%s%d", parent_path,
> +				 PG_TEMP_DIR_TO_REMOVE_PREFIX, n);
> +
> +		if (stat(stage_path, &statbuf) != 0)
> +		{
> +			if (errno == ENOENT)
> +				break;
> +
> +			ereport(LOG,
> +					(errcode_for_file_access(),
> +					 errmsg("could not stat file \"%s\": %m", stage_path)));
> +			return;
> +		}
> +
> +		stage_path[0] = '\0';

I still dislike this approach. Loops until INT_MAX, not interruptible... Can't
we prevent conflicts by adding a timestamp or such?

> +	}
> +
> +	/*
> +	 * In the unlikely event that we couldn't find a name for the stage
> +	 * directory, bail out.
> +	 */
> +	if (stage_path[0] == '\0')
> +	{
> +		ereport(LOG,
> +				(errmsg("could not stage \"%s\" for deletion",
> +						tmp_dir)));
> +		return;
> +	}

That's imo very much not ok. Just continuing in unexpected situations is a
recipe for introducing bugs / being hard to debug.

> From 43042799b96b588a446c509637b5acf570e2a325 Mon Sep 17 00:00:00 2001

> From a58a6bb70785a557a150680b64cd8ce78ce1b73a Mon Sep 17 00:00:00 2001
> From: Nathan Bossart <[email-masked]>
> Date: Sun, 5 Dec 2021 22:02:40 -0800
> Subject: [PATCH v6 5/6] Move removal of old serialized snapshots to custodian.
> 
> This was only done during checkpoints because it was a convenient
> place to put it.

As mentioned before, having it done as part of checkpoints provides pretty
decent wraparound protection - yes, it's not theoretically perfect, but in
reality it's very unlikely you can have an xid wraparound within one
checkpoint. I've mentioned this before, so at the very least I'd like to see
this acknowledged in the commit message.

> However, if there are many snapshots to remove, it can significantly extend
> checkpoint time.

I'd really like to see a reproducer or profile for this...

> +		/*
> +		 * Remove serialized snapshots that are no longer required by any
> +		 * logical replication slot.
> +		 *
> +		 * It is not important for these to be removed in single-user mode, so
> +		 * we don't need any extra handling outside of the custodian process for
> +		 * this.
> +		 */

I don't think this claim is correct.

> From 0add8bb19a4ee83c6a6ec1f313329d737bf304a5 Mon Sep 17 00:00:00 2001
> From: Nathan Bossart <[email-masked]>
> Date: Sun, 12 Dec 2021 22:07:11 -0800
> Subject: [PATCH v6 6/6] Move removal of old logical rewrite mapping files to
>  custodian.
> 
> If there are many such files to remove, checkpoints can take much
> longer.  To avoid this, move this work to the newly-introduced
> custodian process.

As above I'd like to know why this could take that long. What are you doing
that there's so many mapping files (which only exist for catalog tables!) that
this is a significant fraction of a checkpoint?

> ---
>  src/backend/access/heap/rewriteheap.c | 79 +++++++++++++++++++++++----
>  src/backend/postmaster/custodian.c    | 44 +++++++++++++++
>  src/include/access/rewriteheap.h      |  1 +
>  src/include/postmaster/custodian.h    |  5 ++
>  4 files changed, 119 insertions(+), 10 deletions(-)
> 
> diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
> index 2a53826736..edeab65e60 100644
> --- a/src/backend/access/heap/rewriteheap.c
> +++ b/src/backend/access/heap/rewriteheap.c
> @@ -116,6 +116,7 @@
>  #include "lib/ilist.h"
>  #include "miscadmin.h"
>  #include "pgstat.h"
> +#include "postmaster/custodian.h"
>  #include "replication/logical.h"
>  #include "replication/slot.h"
>  #include "storage/bufmgr.h"
> @@ -1182,7 +1183,8 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
>   * Perform a checkpoint for logical rewrite mappings
>   *
>   * This serves two tasks:
> - * 1) Remove all mappings not needed anymore based on the logical restart LSN
> + * 1) Alert the custodian to remove all mappings not needed anymore based on the
> + *    logical restart LSN
>   * 2) Flush all remaining mappings to disk, so that replay after a checkpoint
>   *	  only has to deal with the parts of a mapping that have been written out
>   *	  after the checkpoint started.
> @@ -1210,6 +1212,10 @@ CheckPointLogicalRewriteHeap(void)
>  	if (cutoff != InvalidXLogRecPtr && redo < cutoff)
>  		cutoff = redo;
>  
> +	/* let the custodian know what it can remove */
> +	CustodianSetLogicalRewriteCutoff(cutoff);

Setting this variable in a custodian datastructure and then fetching it from
there seems architecturally wrong to me.

> +	RequestCustodian(CUSTODIAN_REMOVE_REWRITE_MAPPINGS);

What about single user mode?

ISTM that RequestCustodian() needs to either assert out if called in single
user mode, or execute tasks immediately in that context.

> +
> +/*
> + * Remove all mappings not needed anymore based on the logical restart LSN saved
> + * by the checkpointer.  We use this saved value instead of calling
> + * ReplicationSlotsComputeLogicalRestartLSN() so that we don't interfere with an
> + * ongoing call to CheckPointLogicalRewriteHeap() that is flushing mappings to
> + * disk.
> + */

What interference could there be?

> +void
> +RemoveOldLogicalRewriteMappings(void)
> +{
> +	XLogRecPtr	cutoff;
> +	DIR		   *mappings_dir;
> +	struct dirent *mapping_de;
> +	char		path[MAXPGPATH + 20];
> +	bool		value_set = false;
> +
> +	cutoff = CustodianGetLogicalRewriteCutoff(&value_set);
> +	if (!value_set)
> +		return;

Afaics nothing clears values_set - is that a good idea?

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Sun, 3 Jul 2022 10:07:54 -0700

Hi Andres,

Thanks for the prompt review.

> about needing to run the second task.

I think we also want to retry any failed tasks.  The way v6 handles this is
by requesting all tasks after an exception.  Another way to handle this
could be to reset each individual flag before the task is executed, and
then we could surround each one with a PG_CATCH block that resets the flag.
I'll do it this way in the next revision.

> such. And it doesn't seem like a great API for the caller to just specify a

At the moment, the idea is that nothing should need to wait for a task
because the custodian only handles things that are relatively non-critical.
If that changes, this could probably be expanded to look more like
RequestCheckpoint().

What would you suggest using instead of a mix of flags?

> up at that time, or just wake up if somebody requests a task.

I agree.  I will remove the timeout in the next revision.

> This is imo hard to read and easy to get wrong. Make it multiple functions or

Will do.

> staged_for_removal/ directory, and then only scanning that?

Yeah, that seems like a good idea.  Will do.

> we prevent conflicts by adding a timestamp or such?

I suppose it's highly unlikely that we'd see a conflict if we used the
timestamp instead.  I'll do it this way in the next revision if that seems
good enough.

> this acknowledged in the commit message.

Will do.

> there seems architecturally wrong to me.

Where do you think it should go?  I previously had it in the checkpointer's
shared memory, but you didn't like that the functions were declared in
bgwriter.h (along with the other checkpoint stuff).  If the checkpointer
shared memory is the right place, should we create checkpointer.h and use
that instead?

> user mode, or execute tasks immediately in that context.

I like the idea of executing the tasks immediately since that's what
happens today in single-user mode.  I will try doing it that way.

> What interference could there be?

My concern is that the custodian could obtain a later cutoff than what the
checkpointer does, which might cause files to be concurrently unlinked and
fsync'd.  If we always use the checkpointer's cutoff, that shouldn't be a
problem.  This could probably be better explained in this comment.

> Afaics nothing clears values_set - is that a good idea?

I'm using value_set to differentiate the case where InvalidXLogRecPtr means
the checkpointer hasn't determined a value yet versus the case where it
has.  In the former, we don't want to take any action.  In the latter, we
want to unlink all the files.  Since we're moving to a request model for
the custodian, I might be able to remove this value_set stuff completely.
If that's not possible, it probably deserves a better comment.

---

From: Andres Freund <[email-masked]>
Date: Sun, 3 Jul 2022 10:27:32 -0700

Hi,

On 2022-07-03 10:07:54 -0700, Nathan Bossart wrote:
> I think we also want to retry any failed tasks.

I don't think so, at least not if it's just going to retry that task straight
away - then we'll get stuck on that one task forever. If we had the ability to
"queue" it the end, to be processed after other already dequeued tasks, it'd
be a different story.

> The way v6 handles this is by requesting all tasks after an exception.

Ick. That strikes me as a bad idea.

> because the custodian only handles things that are relatively non-critical.

Which is just plainly not true as the patchset stands...

I think we're going to have to block if some cleanup as part of a checkpoint
hasn't been completed by the next checkpoint - otherwise it'll just end up
being way too confusing and there's absolutely no backpressure anymore.

> What would you suggest using instead of a mix of flags?

I suspect an array of tasks with requested and completed counters or such?
With a condition variable to wait on?

> shared memory is the right place, should we create checkpointer.h and use

Well, so far I have not understood what the whole point of the shared state
is, so i have a bit of a hard time answering this ;)

> problem.  This could probably be better explained in this comment.

How about having a Datum argument to RequestCustodian() that is forwarded to
the task?

> If that's not possible, it probably deserves a better comment.

It would.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 6 Jul 2022 09:51:10 -0700

Here's a new revision where I've attempted to address all the feedback I've
received thus far.  Notably, the custodian now uses a queue for registering
tasks and determining which tasks to execute.  Other changes include
splitting the temporary file functions apart to avoid consecutive boolean
flags, using a timestamp instead of an integer for the staging name for
temporary directories, moving temporary directories to a dedicated
directory so that the custodian doesn't need to scan relation files,
ERROR-ing when something goes wrong when cleaning up temporary files,
executing requested tasks immediately in single-user mode, and more.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 11 Aug 2022 16:09:21 -0700

> executing requested tasks immediately in single-user mode, and more.

Here is a rebased patch set for cfbot.  There are no other differences
between v7 and v8.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 24 Aug 2022 09:46:24 -0700

> Here is a rebased patch set for cfbot.  There are no other differences

Another rebase for cfbot.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 Sep 2022 15:07:44 -0700

> Another rebase for cfbot.

And another.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 23 Sep 2022 10:41:54 -0700

> And another.

v11 adds support for building with meson.

---

From: Nathan Bossart <[email-masked]>
Date: Sun, 6 Nov 2022 14:38:42 -0800

> v11 adds support for building with meson.

rebased

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 23 Nov 2022 16:19:07 -0800

> rebased

another rebase for cfbot

---

From: Simon Riggs <[email-masked]>
Date: Thu, 24 Nov 2022 17:31:02 +0000

> On Sun, Nov 06, 2022 at 02:38:42PM -0800, Nathan Bossart wrote:

0001 seems good to me
* I like that it sleeps forever until requested
* not sure I believe that everything it does can always be aborted out
of and shutdown - to achieve that you will need a
CHECK_FOR_INTERRUPTS() calls in the loops in patches 5 and 6 at least
* not sure why you want immediate execution of custodian tasks - I
feel supporting two modes will be a lot harder. For me, I would run
locally when !IsUnderPostmaster and also in an Assert build, so we can
test it works right - i.e. running in its own process is just a
production optimization for performance (which is the stated reason
for having this)

0005 seems good from what I know
* There is no check to see if it worked in any sane time
* It seems possible that "Old" might change meaning - will that make
it break/fail?

0006 seems good also
* same comments for 5

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't really comment with knowledge on sub-patches 0002 to 0004.

Perhaps you should aim to get 1, 5, 6 committed first and then return
to the others in a later CF/separate thread?

---

From: Nathan Bossart <[email-masked]>
Date: Sun, 27 Nov 2022 15:34:34 -0800

Thanks for taking a look!

> CHECK_FOR_INTERRUPTS() calls in the loops in patches 5 and 6 at least

I did something like this earlier, but was advised to simply let the
functions finish as usual during shutdown [0].  I think this is what the
checkpointer process does today, anyway.

> production optimization for performance (which is the stated reason

I added this because 0004 involves requesting a task from the postmaster,
so checking for IsUnderPostmaster doesn't work.  Those tasks would always
run immediately.  However, we could use IsPostmasterEnvironment instead,
which would allow us to remove the "immediate" argument.  I did it this way
in v14.

I'm not sure about running locally in Assert builds.  It's true that would
help ensure there's test coverage for the task logic, but it would also
reduce coverage for the custodian logic.  And in general, I'm worried about
having Assert builds use a different code path than production builds.

> * There is no check to see if it worked in any sane time

What did you have in mind?  Should the custodian begin emitting WARNINGs
after a while?

> * It seems possible that "Old" might change meaning - will that make

I don't believe so.

> 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.  Is the idea just to help with debugging during
the development phase?

> to the others in a later CF/separate thread?

That seems like a good idea since those are all relatively self-contained.
I removed 0002-0004 in v14.

[0] https://postgr.es/m/20220217065938.x2esfdppzypegn5j%40alap3.anarazel.de

---

From: Simon Riggs <[email-masked]>
Date: Mon, 28 Nov 2022 13:08:57 +0000

> checkpointer process does today, anyway.

If we say "The custodian is not an essential process and can shutdown
quickly when requested.", and yet we know its not true in all cases,
then that will lead to misunderstandings and bugs.

If we perform a restart and the custodian is performing extra work
that delays shutdown, then it also delays restart. Given the title of
the thread, we should be looking to improve that, or at least know it
occurred.

> which would allow us to remove the "immediate" argument.  I did it this way

Thanks

> What did you have in mind?  Should the custodian begin emitting WARNINGs

I think it might be useful if it logged anything that took an
"extended period", TBD.

Maybe that is already covered by startup process logging. Please tell
me that still works?

> this sort of thing before.

Much of recovery is coded that way, for the same reason.

> Is the idea just to help with debugging during
> the development phase?

"Just", yes. Tests would be desirable also, under src/test/modules.

---

From: Andres Freund <[email-masked]>
Date: Mon, 28 Nov 2022 10:31:29 -0800

> 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).

---

From: Robert Haas <[email-masked]>
Date: Mon, 28 Nov 2022 13:37:01 -0500

> 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.

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 28 Nov 2022 15:40:39 -0800

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?

> 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.

---

From: Simon Riggs <[email-masked]>
Date: Tue, 29 Nov 2022 12:02:44 +0000

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

That all seems good, thanks.

The last important point for me is tests, in src/test/modules
probably. It might be possible to reuse the final state of other
modules' tests to test cleanup, or at least integrate a custodian test
into each module.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Nov 2022 19:56:53 -0800

> modules' tests to test cleanup, or at least integrate a custodian test

Of course.  I found some existing tests for the test_decoding plugin that
appear to reliably generate the files we want the custodian to clean up, so
I added them there.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Nov 2022 21:18:33 -0800

> appear to reliably generate the files we want the custodian to clean up, so

cfbot is not happy with v16.  AFAICT this is just due to poor placement, so
here is another attempt with the tests moved to a new location.  Apologies
for the noise.

---

From: Simon Riggs <[email-masked]>
Date: Wed, 30 Nov 2022 06:22:49 +0000

> appear to reliably generate the files we want the custodian to clean up, so

Thanks for adding the tests; I can see they run clean.

The only minor thing I would personally add is a note in each piece of
code to explain where the tests are for each one and/or something in
the main custodian file that says tests exist within src/test/module.

Otherwise, ready for committer.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 30 Nov 2022 16:52:44 +0530

> here is another attempt with the tests moved to a new location.  Apologies

Thanks for the patches. I spent some time on reviewing v17 patch set
and here are my comments:

0001:
1. I think the custodian process needs documentation - it needs a
definition in glossary.sgml and perhaps a dedicated page describing
what tasks it takes care of.

2.
+        LWLockReleaseAll();
+        ConditionVariableCancelSleep();
+        AbortBufferIO();
+        UnlockBuffers();
+        ReleaseAuxProcessResources(false);
+        AtEOXact_Buffers(false);
+        AtEOXact_SMgr();
+        AtEOXact_Files(false);
+        AtEOXact_HashTables(false);
Do we need all of these in the exit path? Isn't the stuff that
ShutdownAuxiliaryProcess() does enough for the custodian process?
AFAICS, the custodian process uses LWLocks (which the
ShutdownAuxiliaryProcess() takes care of) and it doesn't access shared
buffers and so on.
Having said that, I'm fine to keep them for future use and all of
those cleanup functions exit if nothing related occurs.

3.
+     * Advertise out latch that backends can use to wake us up while we're
Typo - %s/out/our

4. Is it a good idea to add log messages in the DoCustodianTasks()
loop? Maybe at a debug level? The log message can say the current task
the custodian is processing. And/Or setting the custodian's status on
the ps display is also a good idea IMO.

0002 and 0003:
1.
+CHECKPOINT;
+DO $$
I think we need to ensure that there are some snapshot files before
the checkpoint. Otherwise, it may happen that the above test case
exits without the custodian process doing anything.

2. I think the best way to test the custodian process code is by
adding a TAP test module to see actually the custodian process kicks
in. Perhaps, add elog(DEBUGX,...) messages to various custodian
process functions and see if we see the logs in server logs.

0004:
I think the 0004 patch can be merged into 0001, 0002 and 0003 patches.
Otherwise the patch LGTM.

Few thoughts:
1. I think we can trivially extend the custodian process to remove any
future WAL files on the old timeline, something like the attached
0001-Move-removal-of-future-WAL-files-on-the-old-timeline.text file).
While this offloads the recovery a bit, the server may archive such
WAL files before the custodian removes them. We can do a bit more to
stop the server from archiving such WAL files, but that needs more
coding. I don't think we need to do all that now, perhaps, we can give
it a try once the basic custodian stuff gets in.
2. Moving RemovePgTempFiles() to the custodian can bring up the server
soon. The idea is that the postmaster just renames the temp
directories and informs the custodian so that it can go delete such
temp files and directories. I have personally seen cases where the
server spent a good amount of time cleaning up temp files. We can park
it for later.
3. Moving RemoveOldXlogFiles() to the custodian can make checkpoints faster.
4. PreallocXlogFiles() - if we ever have plans to make pre-allocation
more aggressive (pre-allocate more than 1 WAL file), perhaps letting
custodian do that is a good idea. Again, too many tasks for a single
process.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 30 Nov 2022 17:27:10 +0530

> custodian do that is a good idea. Again, too many tasks for a single

Another comment:
IIUC, there's no custodian_delay GUC as we want to avoid unnecessary
wakeups for power savings (being discussed in the other thread).
However, can it happen that the custodian missed to capture SetLatch
wakeups by other backends? In other words, can the custodian process
be sleeping when there's work to do?

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 1 Dec 2022 13:40:26 -0800

>> and here are my comments:

Thanks for reviewing!

>> what tasks it takes care of.

Good catch.  I added this in v18.  I stopped short of adding a dedicated
page to describe the tasks because 1) there are no parameters for the
custodian and 2) AFAICT none of its tasks are described in the docs today.

>> those cleanup functions exit if nothing related occurs.

Yeah, I don't think we need a few of these.  In v18, I've kept the
following:
	* LWLockReleaseAll()
	* ConditionVariableCancelSleep()
	* ReleaseAuxProcessResources(false)
	* AtEOXact_Files(false)

>> +     * Advertise out latch that backends can use to wake us up while we're

fixed

>> the ps display is also a good idea IMO.

I'd like to pick these up in a new thread if/when this initial patch set is
committed.  The tasks already do some logging, and the checkpointer process
doesn't update the ps display for these tasks today.

>> process functions and see if we see the logs in server logs.

The test appears to reliably create snapshot and mapping files, so if the
directories are empty at some point after the checkpoint at the end, we can
be reasonably certain the custodian took action.  I didn't add explicit
checks that there are files in the directories before the checkpoint
because a concurrent checkpoint could make such checks unreliable.

>> Otherwise the patch LGTM.

I'm keeping this one separate because I've received conflicting feedback
about the idea.

>> custodian do that is a good idea. Again, too many tasks for a single

I definitely want to do #2.  І have some patches for that upthread, but I
removed them for now based on Simon's feedback.  I intend to pick that up
in a new thread.  I haven't thought too much about the others yet.

> be sleeping when there's work to do?

I'm not aware of any way this could happen, but if there is one, I think we
should treat it as a bug instead of relying on the custodian process to
periodically wake up and check for work to do.

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 2 Dec 2022 12:11:35 +0530

> doesn't update the ps display for these tasks today.

It'll be good to have some kind of dedicated monitoring for the
custodian process as it can do a "good" amount of work at times and
users will have a way to know what it currently is doing - it can be
logs at debug level, progress reporting via
ereport_startup_progress()-sort of mechanism, ps display,
pg_stat_custodian or a special function that tells some details, or
some other. In any case, I agree to park this for later.

> because a concurrent checkpoint could make such checks unreliable.

I think you're right. I added sqls to see if the snapshot and mapping
files count > 0, see [1] and the cirrus-ci members are happy too -
https://github.com/BRupireddy/postgres/tree/custodian_review_2. I
think we can consider adding these count > 0 checks to tests.

> I'm keeping this one separate because I've received conflicting feedback

If we classify custodian as a process doing non-critical tasks that
have nothing to do with regular server functioning, then processing
ShutdownRequestPending looks okay. However, delaying these
non-critical tasks such as file removals which reclaims disk space
might impact the server overall especially when it's reaching 100%
disk usage and we want the custodian to do its job fully before we
shutdown the server.

If we delay processing shutdown requests, that can impact the server
overall (might delay restarts, failovers etc.), because at times there
can be a lot of tasks with a good amount of work pending in the
custodian's task queue.

Having said above, I'm okay to process ShutdownRequestPending as early
as possible, however, should we also add CHECK_FOR_INTERRUPTS()
alongside ShutdownRequestPending?

Also, I think it's enough to just have ShutdownRequestPending check in
DoCustodianTasks(void)'s main loop and we can let
RemoveOldSerializedSnapshots() and RemoveOldLogicalRewriteMappings()
do their jobs to the fullest as they do today.

While thinking about this, one thing that really struck me is what
happens if we let the custodian exit, say after processing
ShutdownRequestPending immediately or after a restart, leaving other
queued tasks? The custodian will never get to work on those tasks
unless the requestors (say checkpoint or some other process) requests
it to do so after restart. Maybe, we don't need to worry about it.
Maybe we need to worry about it. Maybe it's an overkill to save the
custodian's task state to disk so that it can come up and do the
leftover tasks upon restart.

> periodically wake up and check for work to do.

One possible scenario is that the requestor adds its task details to
the queue and sets the latch, the custodian can miss this SetLatch()
when it's in the midst of processing a task. However, it guarantees
the requester that it'll process the added task after it completes the
current task. And, I don't know the other reasons when the custodian
can miss SetLatch().

[1]
diff --git a/contrib/test_decoding/expected/rewrite.out
b/contrib/test_decoding/expected/rewrite.out
index 214a514a0a..0029e48852 100644
--- a/contrib/test_decoding/expected/rewrite.out
+++ b/contrib/test_decoding/expected/rewrite.out
@@ -163,6 +163,20 @@ DROP FUNCTION iamalongfunction();
 DROP FUNCTION exec(text);
 DROP ROLE regress_justforcomments;
 -- make sure custodian cleans up files
+-- make sure snapshot files exist for custodian to clean up
+SELECT count(*) > 0 FROM pg_ls_logicalsnapdir();
+ ?column?
+----------
+ t
+(1 row)
+
+-- make sure rewrite mapping files exist for custodian to clean up
+SELECT count(*) > 0 FROM pg_ls_logicalmapdir();
+ ?column?
+----------
+ t
+(1 row)
+
 CHECKPOINT;
 DO $$
 DECLARE
diff --git a/contrib/test_decoding/sql/rewrite.sql
b/contrib/test_decoding/sql/rewrite.sql
index d66f70f837..c076809f37 100644
--- a/contrib/test_decoding/sql/rewrite.sql
+++ b/contrib/test_decoding/sql/rewrite.sql
@@ -107,6 +107,13 @@ DROP FUNCTION exec(text);
 DROP ROLE regress_justforcomments;

 -- make sure custodian cleans up files
+
+-- make sure snapshot files exist for custodian to clean up
+SELECT count(*) > 0 FROM pg_ls_logicalsnapdir();
+
+-- make sure rewrite mapping files exist for custodian to clean up
+SELECT count(*) > 0 FROM pg_ls_logicalmapdir();
+
 CHECKPOINT;
 DO $$
 DECLARE

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 Dec 2022 11:15:07 -0800

> think we can consider adding these count > 0 checks to tests.

My worry about adding "count > 0" checks is that a concurrent checkpoint
could make them unreliable.  In other words, those checks might ordinarily
work, but if an automatic checkpoint causes the files be cleaned up just
beforehand, they will fail.

> alongside ShutdownRequestPending?

I'm not seeing a need for CHECK_FOR_INTERRUPTS.  Do you see one?

> leftover tasks upon restart.

Yes, tasks will need to be retried when the server starts again.  The ones
in this patch set should be requested again during the next checkpoint.
Temporary file cleanup would always be requested during server start, so
that should be handled as well.  Even today, the server might abruptly shut
down while executing these tasks, and we don't have any special handling
for that.

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 6 Dec 2022 12:58:28 +0530

> beforehand, they will fail.

Hm. It would have been better with a TAP test module for testing the
custodian code reliably. Anyway, that mustn't stop the patch getting
in. If required, we can park the TAP test module for later - IMO.
Others may have different thoughts here.

> I'm not seeing a need for CHECK_FOR_INTERRUPTS.  Do you see one?

Since the custodian has SignalHandlerForShutdownRequest as SIGINT and
SIGTERM handlers, unlike StatementCancelHandler and die respectively,
no need of CFI I guess. And also none of the CFI signal handler flags
applies to the custodian.

> down while executing these tasks, and we don't have any special handling

Right.

The v18 patch set posted upthread
https://www.postgresql.org/message-id/20221201214026.GA1799688%40nathanxps13
looks good to me. I see the CF entry is marked RfC -
https://commitfest.postgresql.org/41/3448/.