PATCH: Exclude unlogged tables from base backups

Hi,

On 2017-12-12 17:49:54 -0500, David Steele wrote:

Including unlogged relations in base backups takes up space and is wasteful
since they are truncated during backup recovery.

The attached patches exclude unlogged relations from base backups except for
the init fork, which is required to recreate the main fork during recovery.

How do you reliably identify unlogged relations while writes are going
on? Without locks that sounds, uh, nontrivial?

I decided not to try and document unlogged exclusions in the continuous
backup documentation yet (they are noted in the protocol docs). I would
like to get some input on whether the community thinks this is a good idea.
It's a non-trivial procedure that would be easy to misunderstand and does
not affect the quality of the backup other than using less space. Thoughts?

Think it's a good idea, I've serious concerns about practicability of a
correct implementation though.

- Andres

Hi Andres,

On 12/12/17 5:52 PM, Andres Freund wrote:

On 2017-12-12 17:49:54 -0500, David Steele wrote:

Including unlogged relations in base backups takes up space and is wasteful
since they are truncated during backup recovery.

The attached patches exclude unlogged relations from base backups except for
the init fork, which is required to recreate the main fork during recovery.

How do you reliably identify unlogged relations while writes are going
on? Without locks that sounds, uh, nontrivial?

I don't think this is an issue. If the init fork exists it should be OK
if it is torn since it will be recreated from WAL.

If the forks are written out of order (i.e. main before init), which is
definitely possible, then I think worst case is some files will be
backed up that don't need to be. The main fork is unlikely to be very
large at that point so it doesn't seem like a big deal.

I don't see this as any different than what happens during recovery.
The unlogged forks are cleaned / re-inited before replay starts which is
the same thing we are doing here.

I decided not to try and document unlogged exclusions in the continuous
backup documentation yet (they are noted in the protocol docs). I would
like to get some input on whether the community thinks this is a good idea.
It's a non-trivial procedure that would be easy to misunderstand and does
not affect the quality of the backup other than using less space. Thoughts?

Think it's a good idea, I've serious concerns about practicability of a
correct implementation though.

Well, I would be happy if you had a look!

Thanks.
--
-David
david@pgmasters.net

Hi,

On 2017-12-12 18:04:44 -0500, David Steele wrote:

On 12/12/17 5:52 PM, Andres Freund wrote:

On 2017-12-12 17:49:54 -0500, David Steele wrote:

Including unlogged relations in base backups takes up space and is wasteful
since they are truncated during backup recovery.

The attached patches exclude unlogged relations from base backups except for
the init fork, which is required to recreate the main fork during recovery.

How do you reliably identify unlogged relations while writes are going
on? Without locks that sounds, uh, nontrivial?

I don't think this is an issue. If the init fork exists it should be OK if
it is torn since it will be recreated from WAL.

I'm not worried about torn pages.

If the forks are written out of order (i.e. main before init), which is
definitely possible, then I think worst case is some files will be backed up
that don't need to be. The main fork is unlikely to be very large at that
point so it doesn't seem like a big deal.

I don't see this as any different than what happens during recovery. The
unlogged forks are cleaned / re-inited before replay starts which is the
same thing we are doing here.

It's quite different - in the recovery case there's no other write
activity going on. But on a normally running cluster the persistence of
existing tables can get changed, and oids can get recycled. What
guarantees that between the time you checked for the init fork the table
hasn't been dropped, the oid reused and now a permanent relation is in
its place?

Greetings,

Andres Freund

On Wed, Dec 13, 2017 at 8:04 AM, David Steele <david@pgmasters.net> wrote:

On 12/12/17 5:52 PM, Andres Freund wrote:

On 2017-12-12 17:49:54 -0500, David Steele wrote:

Including unlogged relations in base backups takes up space and is
wasteful
since they are truncated during backup recovery.

The attached patches exclude unlogged relations from base backups except
for
the init fork, which is required to recreate the main fork during
recovery.

How do you reliably identify unlogged relations while writes are going
on? Without locks that sounds, uh, nontrivial?

I don't think this is an issue. If the init fork exists it should be OK if
it is torn since it will be recreated from WAL.

Yeah, I was just typing that until I saw your message.

If the forks are written out of order (i.e. main before init), which is
definitely possible, then I think worst case is some files will be backed up
that don't need to be. The main fork is unlikely to be very large at that
point so it doesn't seem like a big deal.

As far as I recall the init forks are logged before the main forks. I
don't think that we should rely on that assumption though to be always
satisfied.

I decided not to try and document unlogged exclusions in the continuous
backup documentation yet (they are noted in the protocol docs). I would
like to get some input on whether the community thinks this is a good
idea.
It's a non-trivial procedure that would be easy to misunderstand and does
not affect the quality of the backup other than using less space.
Thoughts?

Think it's a good idea, I've serious concerns about practicability of a
correct implementation though.

Well, I would be happy if you had a look!

You can count me in. I think that this patch has value for some
dedicated workloads. It is a waste to backup stuff that will be
removed at recovery anyway.
--
Michael

On 12/12/17 6:07 PM, Andres Freund wrote:

I don't see this as any different than what happens during recovery. The
unlogged forks are cleaned / re-inited before replay starts which is the
same thing we are doing here.

It's quite different - in the recovery case there's no other write
activity going on. But on a normally running cluster the persistence of
existing tables can get changed, and oids can get recycled. What
guarantees that between the time you checked for the init fork the table
hasn't been dropped, the oid reused and now a permanent relation is in
its place?

Well, that's a good point!

How about rechecking the presence of the init fork after a main/other
fork has been found? Is it possible for an init fork to still be lying
around after an oid has been recycled? Seems like it could be...

--
-David
david@pgmasters.net

On 2017-12-12 18:18:09 -0500, David Steele wrote:

On 12/12/17 6:07 PM, Andres Freund wrote:

I don't see this as any different than what happens during recovery. The
unlogged forks are cleaned / re-inited before replay starts which is the
same thing we are doing here.

It's quite different - in the recovery case there's no other write
activity going on. But on a normally running cluster the persistence of
existing tables can get changed, and oids can get recycled. What
guarantees that between the time you checked for the init fork the table
hasn't been dropped, the oid reused and now a permanent relation is in
its place?

Well, that's a good point!

How about rechecking the presence of the init fork after a main/other fork
has been found? Is it possible for an init fork to still be lying around
after an oid has been recycled? Seems like it could be...

I don't see how that'd help. You could just have gone through this cycle
multiple times by the time you get to rechecking. All not very likely,
but I don't want us to rely on luck here...

If we had a way to prevent relfilenode reuse across multiple checkpoints
this'd be easier, although ALTER TABLE SET UNLOGGED still'd complicate.
I guess we could have the basebackup create placeholder files that
prevent relfilenode reuse, but that seems darned ugly.

Greetings,

Andres Freund

Hi Michael,

On 12/12/17 6:08 PM, Michael Paquier wrote:

If the forks are written out of order (i.e. main before init), which is
definitely possible, then I think worst case is some files will be backed up
that don't need to be. The main fork is unlikely to be very large at that
point so it doesn't seem like a big deal.

As far as I recall the init forks are logged before the main forks. I
don't think that we should rely on that assumption though to be always
satisfied.

Indeed, nothing is sure until a checkpoint. Until then we must assume
writes are random.

Well, I would be happy if you had a look!

You can count me in. I think that this patch has value for some
dedicated workloads.

Thanks!

It is a waste to backup stuff that will be
removed at recovery anyway.

It also causes confusion when the recovered database is smaller than the
backup. I can't tell you how many times I have answered this question...

--
-David
david@pgmasters.net

On 12/12/17 6:21 PM, Andres Freund wrote:

On 2017-12-12 18:18:09 -0500, David Steele wrote:

On 12/12/17 6:07 PM, Andres Freund wrote:

It's quite different - in the recovery case there's no other write
activity going on. But on a normally running cluster the persistence of
existing tables can get changed, and oids can get recycled. What
guarantees that between the time you checked for the init fork the table
hasn't been dropped, the oid reused and now a permanent relation is in
its place?

Well, that's a good point!

How about rechecking the presence of the init fork after a main/other fork
has been found? Is it possible for an init fork to still be lying around
after an oid has been recycled? Seems like it could be...

I don't see how that'd help. You could just have gone through this cycle
multiple times by the time you get to rechecking. All not very likely,
but I don't want us to rely on luck here...

Definitely not.

If we had a way to prevent relfilenode reuse across multiple checkpoints
this'd be easier, although ALTER TABLE SET UNLOGGED still'd complicate.

Or error the backup if there is wraparound?

We already have an error if a standby is promoted during backup -- so
there is some precedent.

I guess we could have the basebackup create placeholder files that
prevent relfilenode reuse, but that seems darned ugly.

Yes, very ugly.

--
-David
david@pgmasters.net

Hi,

On 2017-12-12 18:30:47 -0500, David Steele wrote:

If we had a way to prevent relfilenode reuse across multiple checkpoints
this'd be easier, although ALTER TABLE SET UNLOGGED still'd complicate.

Or error the backup if there is wraparound?

That seems entirely unacceptable to me. On a machine with lots of
toasting etc going on an oid wraparound doesn't take a long time. We've
only one oid counter for all tables, and relfilenodes are inferred from
that ....

Greetings,

Andres Freund

On 12/12/17 6:33 PM, Andres Freund wrote:

On 2017-12-12 18:30:47 -0500, David Steele wrote:

If we had a way to prevent relfilenode reuse across multiple checkpoints
this'd be easier, although ALTER TABLE SET UNLOGGED still'd complicate.

Or error the backup if there is wraparound?

That seems entirely unacceptable to me. On a machine with lots of
toasting etc going on an oid wraparound doesn't take a long time. We've
only one oid counter for all tables, and relfilenodes are inferred from
that ....

Fair enough. I'll think on it.

--
-David
david@pgmasters.net

Andres,

* Andres Freund (andres@anarazel.de) wrote:

On 2017-12-12 18:04:44 -0500, David Steele wrote:

If the forks are written out of order (i.e. main before init), which is
definitely possible, then I think worst case is some files will be backed up
that don't need to be. The main fork is unlikely to be very large at that
point so it doesn't seem like a big deal.

I don't see this as any different than what happens during recovery. The
unlogged forks are cleaned / re-inited before replay starts which is the
same thing we are doing here.

It's quite different - in the recovery case there's no other write
activity going on. But on a normally running cluster the persistence of
existing tables can get changed, and oids can get recycled. What
guarantees that between the time you checked for the init fork the table
hasn't been dropped, the oid reused and now a permanent relation is in
its place?

We *are* actually talking about the recovery case here because this is a
backup that's happening and WAL replay will be happening after the
pg_basebackup is done and then the backup restored somewhere and PG
started up again.

If the persistence is changed then the table will be written into the
WAL, no? All of the WAL generated during a backup (which is what we're
talking about here) has to be replayed after the restore is done and is
before the database is considered consistent, so none of this matters,
as far as I can see, because the drop table or alter table logged or
anything else will be in the WAL that ends up getting replayed.

If that's not correct, then isn't there a live issue here with how
backups are happening today with unlogged tables and online backups?

I don't think there is, because, as David points out, the unlogged
tables are cleaned up first and then WAL replay happens during recovery,
so the init fork will cause the relation to be overwritten, but then
later the logged 'drop table' and subsequent re-use of the relfilenode
to create a new table (or persistence change) will all be in the WAL and
will be replayed over top and will take care of this.

Thanks!

Stephen

On 12/12/17 8:48 PM, Stephen Frost wrote:

Andres,

* Andres Freund (andres@anarazel.de) wrote:

On 2017-12-12 18:04:44 -0500, David Steele wrote:

If the forks are written out of order (i.e. main before init), which is
definitely possible, then I think worst case is some files will be backed up
that don't need to be. The main fork is unlikely to be very large at that
point so it doesn't seem like a big deal.

I don't see this as any different than what happens during recovery. The
unlogged forks are cleaned / re-inited before replay starts which is the
same thing we are doing here.

It's quite different - in the recovery case there's no other write
activity going on. But on a normally running cluster the persistence of
existing tables can get changed, and oids can get recycled. What
guarantees that between the time you checked for the init fork the table
hasn't been dropped, the oid reused and now a permanent relation is in
its place?

We *are* actually talking about the recovery case here because this is a
backup that's happening and WAL replay will be happening after the
pg_basebackup is done and then the backup restored somewhere and PG
started up again.

If the persistence is changed then the table will be written into the
WAL, no? All of the WAL generated during a backup (which is what we're
talking about here) has to be replayed after the restore is done and is
before the database is considered consistent, so none of this matters,
as far as I can see, because the drop table or alter table logged or
anything else will be in the WAL that ends up getting replayed.

Yes - that's the way I see it. At least when I'm not tired from a day
of coding like I was last night...

I don't think there is, because, as David points out, the unlogged
tables are cleaned up first and then WAL replay happens during recovery,
so the init fork will cause the relation to be overwritten, but then
later the logged 'drop table' and subsequent re-use of the relfilenode
to create a new table (or persistence change) will all be in the WAL and
will be replayed over top and will take care of this.

Files can be copied in any order, so if an OID is recycled the backup
could copy its first, second, or nth incarnation. It doesn't really
matter since all of it will be clobbered by WAL replay.

The new base backup code just does the non-init fork removal in advance,
following the same rules that would apply on recovery given the same
file set.

--
-David
david@pgmasters.net

David,

* David Steele (david@pgmasters.net) wrote:

On 12/12/17 8:48 PM, Stephen Frost wrote:

I don't think there is, because, as David points out, the unlogged
tables are cleaned up first and then WAL replay happens during recovery,
so the init fork will cause the relation to be overwritten, but then
later the logged 'drop table' and subsequent re-use of the relfilenode
to create a new table (or persistence change) will all be in the WAL and
will be replayed over top and will take care of this.

Files can be copied in any order, so if an OID is recycled the backup
could copy its first, second, or nth incarnation. It doesn't really
matter since all of it will be clobbered by WAL replay.

The new base backup code just does the non-init fork removal in advance,
following the same rules that would apply on recovery given the same
file set.

Just to be clear- the new base backup code doesn't actually *do* the
non-init fork removal, it simply doesn't include the non-init fork in
the backup when there is an init fork, right?

We certainly wouldn't want a basebackup actually running around removing
the main fork for unlogged tables on a running and otherwise healthy
system. ;)

Thanks!

Stephen

On 12/13/17 10:04 AM, Stephen Frost wrote:

Just to be clear- the new base backup code doesn't actually *do* the
non-init fork removal, it simply doesn't include the non-init fork in
the backup when there is an init fork, right?

It does *not* do the unlogged non-init fork removal. The code I
refactored in reinit.c is about identifying the forks, not removing
them. That code is reused to determine what to exclude from the backup.

I added the regression tests to ensure that the behavior of reinit.c is
unchanged after the refactor.

We certainly wouldn't want a basebackup actually running around removing
the main fork for unlogged tables on a running and otherwise healthy
system. ;)

That would not be good.

--
-David
david@pgmasters.net

On Tue, Dec 12, 2017 at 8:48 PM, Stephen Frost <sfrost@snowman.net> wrote:

If the persistence is changed then the table will be written into the
WAL, no? All of the WAL generated during a backup (which is what we're
talking about here) has to be replayed after the restore is done and is
before the database is considered consistent, so none of this matters,
as far as I can see, because the drop table or alter table logged or
anything else will be in the WAL that ends up getting replayed.

I can't see a hole in this argument. If we copy the init fork and
skip copying the main fork, then either we skipped copying the right
file, or the file we skipped copying will be recreated with the
correct contents during WAL replay anyway.

We could have a problem if wal_level=minimal, because then the new
file might not have been WAL-logged; but taking an online backup with
wal_level=minimal isn't supported precisely because we won't have WAL
replay to fix things up.

We would also have a problem if the missing file caused something in
recovery to croak on the grounds that the file was expected to be
there, but I don't think anything works that way; I think we just
assume missing files are an expected failure mode and silently do
nothing if asked to remove them.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

All,

I have reviewed and tested these patches.

The patches applied cleanly in order against master at (90947674fc).

I ran the provided regression tests and a 'check-world'. All tests succeeded.

Marking ready for committer.

-Adam

On Thu, Dec 14, 2017 at 11:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Dec 12, 2017 at 8:48 PM, Stephen Frost <sfrost@snowman.net> wrote:

If the persistence is changed then the table will be written into the
WAL, no? All of the WAL generated during a backup (which is what we're
talking about here) has to be replayed after the restore is done and is
before the database is considered consistent, so none of this matters,
as far as I can see, because the drop table or alter table logged or
anything else will be in the WAL that ends up getting replayed.

I can't see a hole in this argument. If we copy the init fork and
skip copying the main fork, then either we skipped copying the right
file, or the file we skipped copying will be recreated with the
correct contents during WAL replay anyway.

We could have a problem if wal_level=minimal, because then the new
file might not have been WAL-logged; but taking an online backup with
wal_level=minimal isn't supported precisely because we won't have WAL
replay to fix things up.

We would also have a problem if the missing file caused something in
recovery to croak on the grounds that the file was expected to be
there, but I don't think anything works that way; I think we just
assume missing files are an expected failure mode and silently do
nothing if asked to remove them.

I also couldn't see a problem in this approach.

Here is the first review comments.

+ unloggedDelim = strrchr(path, '/');

I think it doesn't work fine on windows. How about using
last_dir_separator() instead?

----
+ * Find all unlogged relations in the specified directory and return
their OIDs.

What the ResetUnloggedrelationsHash() actually returns is a hash
table. The comment of this function seems not appropriate.

----
+               /* Part of path that contains the parent directory. */
+               int parentPathLen = unloggedDelim - path;
+
+               /*
+                * Build the unlogged relation hash if the parent path is either
+                * $PGDATA/base or a tablespace version path.
+                */
+               if (strncmp(path, "./base", parentPathLen) == 0 ||
+                       (parentPathLen >=
(sizeof(TABLESPACE_VERSION_DIRECTORY) - 1) &&
+                        strncmp(unloggedDelim -
(sizeof(TABLESPACE_VERSION_DIRECTORY) - 1),
+                                        TABLESPACE_VERSION_DIRECTORY,
+
sizeof(TABLESPACE_VERSION_DIRECTORY) - 1) == 0))
+                       unloggedHash = ResetUnloggedRelationsHash(path);
+       }

How about using get_parent_directory() to get parent directory name?
Also, I think it's better to destroy the unloggedHash after use.

----
+               /* Exclude all forks for unlogged tables except the
init fork. */
+               if (unloggedHash && ResetUnloggedRelationsMatch(
+                               unloggedHash, de->d_name) == unloggedOther)
+               {
+                       elog(DEBUG2, "unlogged relation file \"%s\"
excluded from backup",
+                                de->d_name);
+                       continue;
+               }

I think it's better to log this debug message at DEBUG2 level for
consistency with other messages.

----
+       ok(!-f "$tempdir/tbackup/tblspc1/$tblspc1UnloggedBackupPath",
+               'unlogged imain fork not in tablespace backup');

s/imain/main/

----
If a new unlogged relation is created after constructed the
unloggedHash before sending file, we cannot exclude such relation. It
would not be problem if the taking backup is not long because the new
unlogged relation unlikely becomes so large. However, if takeing a
backup takes a long time, we could include large main fork in the
backup.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hi Masahiko,

Thanks for the review!

On 1/22/18 3:14 AM, Masahiko Sawada wrote:

On Thu, Dec 14, 2017 at 11:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:

We would also have a problem if the missing file caused something in
recovery to croak on the grounds that the file was expected to be
there, but I don't think anything works that way; I think we just
assume missing files are an expected failure mode and silently do
nothing if asked to remove them.

I also couldn't see a problem in this approach.

Here is the first review comments.

+ unloggedDelim = strrchr(path, '/');

I think it doesn't work fine on windows. How about using
last_dir_separator() instead?

I think this function is OK on Windows -- we use it quite a bit.
However, last_dir_separator() is clearer so I have changed it.

----
+ * Find all unlogged relations in the specified directory and return
their OIDs.

What the ResetUnloggedrelationsHash() actually returns is a hash
table. The comment of this function seems not appropriate.

Fixed.

+               /* Part of path that contains the parent directory. */
+               int parentPathLen = unloggedDelim - path;
+
+               /*
+                * Build the unlogged relation hash if the parent path is either
+                * $PGDATA/base or a tablespace version path.
+                */
+               if (strncmp(path, "./base", parentPathLen) == 0 ||
+                       (parentPathLen >=
(sizeof(TABLESPACE_VERSION_DIRECTORY) - 1) &&
+                        strncmp(unloggedDelim -
(sizeof(TABLESPACE_VERSION_DIRECTORY) - 1),
+                                        TABLESPACE_VERSION_DIRECTORY,
+
sizeof(TABLESPACE_VERSION_DIRECTORY) - 1) == 0))
+                       unloggedHash = ResetUnloggedRelationsHash(path);
+       }

How about using get_parent_directory() to get parent directory name?

get_parent_directory() munges the string that is passed to it which I
was trying to avoid (we'd need a copy) - and I don't think it makes the
rest of the logic any simpler without constructing yet another string to
hold the tablespace path.

I know performance isn't the most important thing here, so if the
argument is for clarity perhaps it makes sense. Otherwise I don't know
if it's worth it.

Also, I think it's better to destroy the unloggedHash after use.

Whoops! Fixed.

+               /* Exclude all forks for unlogged tables except the
init fork. */
+               if (unloggedHash && ResetUnloggedRelationsMatch(
+                               unloggedHash, de->d_name) == unloggedOther)
+               {
+                       elog(DEBUG2, "unlogged relation file \"%s\"
excluded from backup",
+                                de->d_name);
+                       continue;
+               }

I think it's better to log this debug message at DEBUG2 level for
consistency with other messages.

I think you mean DEBUG1? It's already at DEBUG2.

I considered using DEBUG1 but decided against it. The other exclusions
will produce a limited amount of output because there are only a few of
them. In the case of unlogged tables there could be any number of
exclusions and I thought that was too noisy for DEBUG1.

+       ok(!-f "$tempdir/tbackup/tblspc1/$tblspc1UnloggedBackupPath",
+               'unlogged imain fork not in tablespace backup');

s/imain/main/

Fixed.

If a new unlogged relation is created after constructed the
unloggedHash before sending file, we cannot exclude such relation. It
would not be problem if the taking backup is not long because the new
unlogged relation unlikely becomes so large. However, if takeing a
backup takes a long time, we could include large main fork in the
backup.

This is a good point. It's per database directory which makes it a
little better, but maybe not by much.

Three options here:

1) Leave it as is knowing that unlogged relations created during the
backup may be copied and document it that way.

2) Construct a list for SendDir() to work against so the gap between
creating that and creating the unlogged hash is as small as possible.
The downside here is that the list may be very large and take up a lot
of memory.

3) Check each file that looks like a relation in the loop to see if it
has an init fork. This might affect performance since an
opendir/readdir loop would be required for every relation.

Personally, I'm in favor of #1, at least for the time being. I've
updated the docs as indicated in case you and Adam agree.

New patches attached.

Thanks!
--
-David
david@pgmasters.net

If a new unlogged relation is created after constructed the
unloggedHash before sending file, we cannot exclude such relation. It
would not be problem if the taking backup is not long because the new
unlogged relation unlikely becomes so large. However, if takeing a
backup takes a long time, we could include large main fork in the
backup.

This is a good point. It's per database directory which makes it a
little better, but maybe not by much.

Three options here:

1) Leave it as is knowing that unlogged relations created during the
backup may be copied and document it that way.

2) Construct a list for SendDir() to work against so the gap between
creating that and creating the unlogged hash is as small as possible.
The downside here is that the list may be very large and take up a lot
of memory.

3) Check each file that looks like a relation in the loop to see if it
has an init fork. This might affect performance since an
opendir/readdir loop would be required for every relation.

Personally, I'm in favor of #1, at least for the time being. I've
updated the docs as indicated in case you and Adam agree.

I agree with #1 and feel the updated docs are reasonable and
sufficient to address this case for now.

I have retested these patches against master at d6ab720360.

All test succeed.

Marking "Ready for Committer".

-Adam