Feedback-ID: ia2694551:Fastmail
Date: Thu, 16 Nov 2023 18:23:00 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>,
	Andres Freund <andres@anarazel.de>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: trying again to get incremental backup
Message-ID: <202311161723.cffoiarzi5fl@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+TgmoYw=8CnKEPQX9qk2estSTCaB7OGcnVM8oa-hAePG2O4Yw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202311161723.cffoiarzi5fl%40alvherre.pgsql>
Precedence: bulk

On 2023-Oct-04, Robert Haas wrote:

> - I would like some feedback on the generation of WAL summary files.
> Right now, I have it enabled by default, and summaries are kept for a
> week. That means that, with no additional setup, you can take an
> incremental backup as long as the reference backup was taken in the
> last week. File removal is governed by mtimes, so if you change the
> mtimes of your summary files or whack your system clock around, weird
> things might happen. But obviously this might be inconvenient. Some
> people might not want WAL summary files to be generated at all because
> they don't care about incremental backup, and other people might want
> them retained for longer, and still other people might want them to be
> not removed automatically or removed automatically based on some
> criteria other than mtime. I don't really know what's best here. I
> don't think the default policy that the patches implement is
> especially terrible, but it's just something that I made up and I
> don't have any real confidence that it's wonderful. One point to be
> consider here is that, if WAL summarization is enabled, checkpoints
> can't remove WAL that isn't summarized yet. Mostly that's not a
> problem, I think, because the WAL summarizer is pretty fast. But it
> could increase disk consumption for some people. I don't think that we
> need to worry about the summaries themselves being a problem in terms
> of space consumption; at least in all the cases I've tested, they're
> just not very big.

So, wal_summary is no longer turned on by default, I think following a
comment from Peter E.  I think this is a good decision, as we're only
going to need them on servers from which incremental backups are going
to be taken, which is a strict subset of all servers; and furthermore,
people that need them are going to realize that very easily, while if we
went the other around most people would not realize that they need to
turn them off to save some resource consumption.

Granted, the amount of resources additionally used is probably not very
big.  But since it can be changed with a reload not restart, it doesn't
seem problematic.

... oh, I just noticed that this patch now fails to compile because of
the MemoryContextResetAndDeleteChildren removal.

(Typo in the pg_walsummary manpage: "since WAL summary files primary
exist" -> "primarily")

> - On a related note, I haven't yet tested this on a standby, which is
> a thing that I definitely need to do. I don't know of a reason why it
> shouldn't be possible for all of this machinery to work on a standby
> just as it does on a primary, but then we need the WAL summarizer to
> run there too, which could end up being a waste if nobody ever tries
> to take an incremental backup. I wonder how that should be reflected
> in the configuration. We could do something like what we've done for
> archive_mode, where on means "only on if this is a primary" and you
> have to say always if you want it to run on standbys as well ... but
> I'm not sure if that's a design pattern that we really want to
> replicate into more places. I'd be somewhat inclined to just make
> whatever configuration parameters we need to configure this thing on
> the primary also work on standbys, and you can set each server up as
> you please. But I'm open to other suggestions.

I think it should default to off in primary and standby, and the user
has to enable it in whichever server they want to take backups from.

> - We need to settle the question of whether to send the whole backup
> manifest to the server or just the LSN. In a previous attempt at
> incremental backup, we decided the whole manifest was necessary,
> because flat-copying files could make new data show up with old LSNs.
> But that version of the patch set was trying to find modified blocks
> by checking their LSNs individually, not by summarizing WAL. And since
> the operations that flat-copy files are WAL-logged, the WAL summary
> approach seems to eliminate that problem - maybe an LSN (and the
> associated TLI) is good enough now. This also relates to Jakub's
> question about whether this machinery could be used to fast-forward a
> standby, which is not exactly a base backup but  ... perhaps close
> enough? I'm somewhat inclined to believe that we can simplify to an
> LSN and TLI; however, if we do that, then we'll have big problems if
> later we realize that we want the manifest for something after all. So
> if anybody thinks that there's a reason to keep doing what the patch
> does today -- namely, upload the whole manifest to the server --
> please speak up.

I don't understand this point.  Currently, the protocol is that
UPLOAD_MANIFEST is used to send the manifest prior to requesting the
backup.  You seem to be saying that you're thinking of removing support
for UPLOAD_MANIFEST and instead just give the LSN as an option to the
BASE_BACKUP command?

> - It's regrettable that we don't have incremental JSON parsing;

We now do have it, at least in patch form.  I guess the question is
whether we're going to accept it in core.  I see chances of changing the
format of the manifest rather slim at this point, and the need for very
large manifests is likely to go up with time, so we probably need to
take that code and polish it up, and see if we can improve its
performance.

> - Right now, I have a hard-coded 60 second timeout for WAL
> summarization. If you try to take an incremental backup and the WAL
> summaries you need don't show up within 60 seconds, the backup times
> out. I think that's a reasonable default, but should it be
> configurable? If yes, should that be a GUC or, perhaps better, a
> pg_basebackup option?

I'd rather have a way for the server to provide diagnostics on why the
summaries aren't being produced.  Maybe a server running under valgrind
is going to fail and need a longer one, but otherwise a hardcoded
timeout seems sufficient.

You did say later that you thought summary files would just go from one
checkpoint to the next.  So the only question is at what point the file
for the last checkpoint (i.e. from the previous one up to the one
requested by pg_basebackup) is written.  If walsummarizer keeps almost
the complete state in memory and just waits for the checkpoint record to
write it, then it's probably okay.

> - I'm curious what people think about the pg_walsummary tool that is
> included in 0006. I think it's going to be fairly important for
> debugging, but it does feel a little bit bad to add a new binary for
> something pretty niche. Nevertheless, merging it into any other
> utility seems relatively awkward, so I'm inclined to think both that
> this should be included in whatever finally gets committed and that it
> should be a separate binary. I considered whether it should go in
> contrib, but we seem to have moved to a policy that heavily favors
> limiting contrib to extensions and loadable modules, rather than
> binaries.

I propose to keep the door open for that binary doing other things that
dumping the files as text.  So add a command argument, which currently
can only be "dump", to allow the command do other things later if
needed.  (For example, remove files from a server on which summarize_wal
has been turned off; or perhaps remove files that are below some LSN.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Estoy de acuerdo contigo en que la verdad absoluta no existe...
El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)