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 <robertmh...@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 da...@pgmasters.net
diff --git a/src/test/recovery/t/014_unlogged_reinit.pl b/src/test/recovery/t/014_unlogged_reinit.pl new file mode 100644 index 0000000000..ac2e251158 --- /dev/null +++ b/src/test/recovery/t/014_unlogged_reinit.pl @@ -0,0 +1,117 @@ +# Tests that unlogged tables are properly reinitialized after a crash. +# +# The behavior should be the same when restoring from a backup but that is not +# tested here (yet). +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More tests => 16; + +# Initialize node without replication settings +my $node = get_new_node('main'); + +$node->init; +$node->start; +my $pgdata = $node->data_dir; + +# Create an unlogged table to test that forks other than init are not copied +$node->safe_psql('postgres', 'CREATE UNLOGGED TABLE base_unlogged (id int)'); + +my $baseUnloggedPath = $node->safe_psql('postgres', + q{select pg_relation_filepath('base_unlogged')}); + +# Make sure main and init forks exist +ok(-f "$pgdata/${baseUnloggedPath}_init", 'init fork in base'); +ok(-f "$pgdata/$baseUnloggedPath", 'main fork in base'); + +# The following tests test symlinks. Windows doesn't have symlinks, so +# skip on Windows. +my $tablespaceDir = undef; +my $ts1UnloggedPath = undef; + +SKIP: +{ + skip "symlinks not supported on Windows", 2 if ($windows_os); + + # Create unlogged tables in a tablespace + $tablespaceDir = TestLib::tempdir . "/ts1"; + + mkdir($tablespaceDir) + or die "unable to mkdir \"$tablespaceDir\""; + + $node->safe_psql('postgres', + "CREATE TABLESPACE ts1 LOCATION '$tablespaceDir'"); + $node->safe_psql('postgres', + 'CREATE UNLOGGED TABLE ts1_unlogged (id int) TABLESPACE ts1'); + + $ts1UnloggedPath = $node->safe_psql('postgres', + q{select pg_relation_filepath('ts1_unlogged')}); + + # Make sure main and init forks exist + ok(-f "$pgdata/${ts1UnloggedPath}_init", 'init fork in tablespace'); + ok(-f "$pgdata/$ts1UnloggedPath", 'main fork in tablespace'); +} + +# Crash the postmaster +$node->stop('immediate'); + +# Write forks to test that they are removed during recovery +$node->command_ok(['touch', "$pgdata/${baseUnloggedPath}_vm"], + 'touch vm fork in base'); +$node->command_ok(['touch', "$pgdata/${baseUnloggedPath}_fsm"], + 'touch fsm fork in base'); + +# Remove main fork to test that it is recopied from init +unlink("$pgdata/${baseUnloggedPath}") + or die "unable to remove \"${baseUnloggedPath}\""; + +# The following tests test symlinks. Windows doesn't have symlinks, so +# skip on Windows. +SKIP: +{ + skip "symlinks not supported on Windows", 2 if ($windows_os); + + # Write forks to test that they are removed by recovery + $node->command_ok(['touch', "$pgdata/${ts1UnloggedPath}_vm"], + 'touch vm fork in tablespace'); + $node->command_ok(['touch', "$pgdata/${ts1UnloggedPath}_fsm"], + 'touch fsm fork in tablespace'); + + # Remove main fork to test that it is recopied from init + unlink("$pgdata/${ts1UnloggedPath}") + or die "unable to remove \"${ts1UnloggedPath}\""; +} + +# Start the postmaster +$node->start; + +# Check unlogged table in base +ok(-f "$pgdata/${baseUnloggedPath}_init", 'init fork in base'); +ok(-f "$pgdata/$baseUnloggedPath", 'main fork in base'); +ok(!-f "$pgdata/${baseUnloggedPath}_vm", 'vm fork not in base'); +ok(!-f "$pgdata/${baseUnloggedPath}_fsm", 'fsm fork not in base'); + +# Drop unlogged table +$node->safe_psql('postgres', 'DROP TABLE base_unlogged'); + +# The following tests test symlinks. Windows doesn't have symlinks, so +# skip on Windows. +SKIP: +{ + skip "symlinks not supported on Windows", 4 if ($windows_os); + + # Check unlogged table in tablespace + ok(-f "$pgdata/${ts1UnloggedPath}_init", 'init fork in tablespace'); + ok(-f "$pgdata/$ts1UnloggedPath", 'main fork in tablespace'); + ok(!-f "$pgdata/${ts1UnloggedPath}_vm", 'vm fork not in tablespace'); + ok(!-f "$pgdata/${ts1UnloggedPath}_fsm", 'fsm fork not in tablespace'); + + # Drop unlogged table + $node->safe_psql('postgres', 'DROP TABLE ts1_unlogged'); + + # Drop tablespace + $node->safe_psql('postgres', 'DROP TABLESPACE ts1'); + rmdir($tablespaceDir) + or die "unable to rmdir \"$tablespaceDir\""; +}
diff --git a/src/backend/storage/file/reinit.c b/src/backend/storage/file/reinit.c index 92363ae6ad..73a25fa3e7 100644 --- a/src/backend/storage/file/reinit.c +++ b/src/backend/storage/file/reinit.c @@ -21,7 +21,6 @@ #include "storage/copydir.h" #include "storage/fd.h" #include "storage/reinit.h" -#include "utils/hsearch.h" #include "utils/memutils.h" static void ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, @@ -145,6 +144,100 @@ ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op) FreeDir(ts_dir); } +/* + * Find all unlogged relations in the specified directory and return their OIDs + * as a hash table. + * + * It's possible that someone could create a ton of unlogged relations in the + * same database & tablespace, so we'd better use a hash table rather than an + * array or linked list to keep track of which files need to be reset. + * Otherwise, search operations would be O(n^2). + */ +HTAB * +ResetUnloggedRelationsHash(const char *dbspacedirname) +{ + DIR *dbspace_dir; + struct dirent *de; + HTAB *hash = NULL; + + /* Scan the directory. */ + dbspace_dir = AllocateDir(dbspacedirname); + while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL) + { + ForkNumber forkNum; + int oidchars; + unlogged_relation_entry ent; + + /* Skip anything that doesn't look like a relation data file. */ + if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars, + &forkNum)) + continue; + + /* Also skip it unless this is the init fork. */ + if (forkNum != INIT_FORKNUM) + continue; + + /* Create the hash table if it has not been created already. */ + if (!hash) + { + HASHCTL ctl; + + memset(&ctl, 0, sizeof(ctl)); + ctl.keysize = sizeof(unlogged_relation_entry); + ctl.entrysize = sizeof(unlogged_relation_entry); + hash = hash_create("unlogged hash", 32, &ctl, HASH_ELEM); + } + + /* + * Put the OID portion of the name into the hash table, if it + * isn't already. + */ + memset(ent.oid, 0, sizeof(ent.oid)); + memcpy(ent.oid, de->d_name, oidchars); + hash_search(hash, &ent, HASH_ENTER, NULL); + } + + /* Done with the first pass. */ + FreeDir(dbspace_dir); + + return hash; +} + +/* + * Determine whether the specified file is an unlogged relation fork. + * + * If not an unlogged relation then return notUnlogged, otherwise return + * unloggedInit if an unlogged init fork and unloggedOther if any other unlogged + * fork. + */ +UnloggedRelationFork +ResetUnloggedRelationsMatch(HTAB *hash, const char *file) +{ + ForkNumber forkNum; + int oidchars; + bool found; + unlogged_relation_entry ent; + + /* If it's not a relation then it's not unlogged. */ + if (!parse_filename_for_nontemp_relation(file, &oidchars, &forkNum)) + return notUnlogged; + + /* An unlogged init fork. */ + if (forkNum == INIT_FORKNUM) + return unloggedInit; + + /* See whether the OID portion of the name shows up in the hash table. */ + memset(ent.oid, 0, sizeof(ent.oid)); + memcpy(ent.oid, file, oidchars); + hash_search(hash, &ent, HASH_FIND, &found); + + /* If found this is another fork of an unlogged table (but not init). */ + if (found) + return unloggedOther; + + return notUnlogged; +} + /* * Process one per-dbspace directory for ResetUnloggedRelations */ @@ -166,58 +259,13 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op) if ((op & UNLOGGED_RELATION_CLEANUP) != 0) { HTAB *hash; - HASHCTL ctl; - - /* - * It's possible that someone could create a ton of unlogged relations - * in the same database & tablespace, so we'd better use a hash table - * rather than an array or linked list to keep track of which files - * need to be reset. Otherwise, this cleanup operation would be - * O(n^2). - */ - memset(&ctl, 0, sizeof(ctl)); - ctl.keysize = sizeof(unlogged_relation_entry); - ctl.entrysize = sizeof(unlogged_relation_entry); - hash = hash_create("unlogged hash", 32, &ctl, HASH_ELEM); - - /* Scan the directory. */ - dbspace_dir = AllocateDir(dbspacedirname); - while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL) - { - ForkNumber forkNum; - int oidchars; - unlogged_relation_entry ent; - - /* Skip anything that doesn't look like a relation data file. */ - if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars, - &forkNum)) - continue; - - /* Also skip it unless this is the init fork. */ - if (forkNum != INIT_FORKNUM) - continue; - /* - * Put the OID portion of the name into the hash table, if it - * isn't already. - */ - memset(ent.oid, 0, sizeof(ent.oid)); - memcpy(ent.oid, de->d_name, oidchars); - hash_search(hash, &ent, HASH_ENTER, NULL); - } - - /* Done with the first pass. */ - FreeDir(dbspace_dir); + /* Build a hash table of all unlogged relations. */ + hash = ResetUnloggedRelationsHash(dbspacedirname); - /* - * If we didn't find any init forks, there's no point in continuing; - * we can bail out now. - */ - if (hash_get_num_entries(hash) == 0) - { - hash_destroy(hash); + /* No need to continue if there are no unlogged tables. */ + if (!hash) return; - } /* * Now, make a second pass and remove anything that matches. @@ -225,30 +273,9 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op) dbspace_dir = AllocateDir(dbspacedirname); while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL) { - ForkNumber forkNum; - int oidchars; - bool found; - unlogged_relation_entry ent; - - /* Skip anything that doesn't look like a relation data file. */ - if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars, - &forkNum)) - continue; - - /* We never remove the init fork. */ - if (forkNum == INIT_FORKNUM) - continue; - - /* - * See whether the OID portion of the name shows up in the hash - * table. - */ - memset(ent.oid, 0, sizeof(ent.oid)); - memcpy(ent.oid, de->d_name, oidchars); - hash_search(hash, &ent, HASH_FIND, &found); - - /* If so, nuke it! */ - if (found) + /* If this is an unlogged relation fork other than init, nuke it! */ + if (ResetUnloggedRelationsMatch( + hash, de->d_name) == unloggedOther) { snprintf(rm_path, sizeof(rm_path), "%s/%s", dbspacedirname, de->d_name); diff --git a/src/include/storage/reinit.h b/src/include/storage/reinit.h index addda2c0ea..cd422e519a 100644 --- a/src/include/storage/reinit.h +++ b/src/include/storage/reinit.h @@ -15,9 +15,24 @@ #ifndef REINIT_H #define REINIT_H +#include "utils/hsearch.h" + extern void ResetUnloggedRelations(int op); #define UNLOGGED_RELATION_CLEANUP 0x0001 #define UNLOGGED_RELATION_INIT 0x0002 +/* Return values for ResetUnloggedRelationsMatch(). */ +typedef enum +{ + notUnlogged, /* Not a relation or not an unlogged relation. */ + unloggedInit, /* An unlogged relation init fork. */ + unloggedOther /* An unlogged relation fork other than init. */ +} UnloggedRelationFork; + +/* Utility functions for identifying unlogged table forks. */ +extern HTAB *ResetUnloggedRelationsHash(const char *dbspacedirname); +extern UnloggedRelationFork ResetUnloggedRelationsMatch( + HTAB *unloggedHash, const char *fileName); + #endif /* REINIT_H */
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 4c5ed1e6d6..3fec491bbe 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -2552,6 +2552,14 @@ The commands accepted in walsender mode are: with <filename>pgsql_tmp</filename>. </para> </listitem> + <listitem> + <para> + Unlogged relations, except for the init fork which is required to + recreate the (empty) unlogged relation on recovery. Unlogged tables + created during a backup may be copied, but will be excluded from the + next backup. + </para> + </listitem> <listitem> <para> <filename>pg_wal</filename>, including subdirectories. If the backup is run diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index dd7ad64862..1a5c91de17 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -26,6 +26,7 @@ #include "nodes/pg_list.h" #include "pgtar.h" #include "pgstat.h" +#include "port.h" #include "postmaster/syslogger.h" #include "replication/basebackup.h" #include "replication/walsender.h" @@ -33,6 +34,7 @@ #include "storage/dsm_impl.h" #include "storage/fd.h" #include "storage/ipc.h" +#include "storage/reinit.h" #include "utils/builtins.h" #include "utils/elog.h" #include "utils/ps_status.h" @@ -959,6 +961,36 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces, char pathbuf[MAXPGPATH * 2]; struct stat statbuf; int64 size = 0; + HTAB *unloggedHash = NULL; /* Unlogged tables in this path. */ + const char *unloggedDelim; /* Split this path from parent path. */ + + /* + * Find any unlogged relations in this path and store them in a hash. All + * unlogged relation forks except init will be excluded from the backup. + * + * Start by finding the location of the delimiter between the parent + * path and the current path. + */ + unloggedDelim = last_dir_separator(path); + + /* Does this path look like a database path (i.e. all digits)? */ + if (unloggedDelim != NULL && + strspn(unloggedDelim + 1, "0123456789") == strlen(unloggedDelim + 1)) + { + /* 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); + } dir = AllocateDir(path); while ((de = ReadDir(dir, path)) != NULL) @@ -1008,6 +1040,15 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces, if (excludeFound) continue; + /* 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; + } + snprintf(pathbuf, sizeof(pathbuf), "%s/%s", path, de->d_name); /* Skip pg_control here to back up it last */ @@ -1177,6 +1218,11 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces, (errmsg("skipping special file \"%s\"", pathbuf))); } FreeDir(dir); + + /* Free the hash containing unlogged tables, if it was created */ + if (unloggedHash != NULL) + hash_destroy(unloggedHash); + return size; } diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index cdf4f5be37..455c7fca0d 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -4,7 +4,7 @@ use Cwd; use Config; use PostgresNode; use TestLib; -use Test::More tests => 79; +use Test::More tests => 87; program_help_ok('pg_basebackup'); program_version_ok('pg_basebackup'); @@ -66,6 +66,16 @@ foreach my $filename ( # positive. $node->safe_psql('postgres', 'SELECT 1;'); +# Create an unlogged table to test that forks other than init are not copied. +$node->safe_psql('postgres', 'CREATE UNLOGGED TABLE base_unlogged (id int)'); + +my $baseUnloggedPath = $node->safe_psql('postgres', + q{select pg_relation_filepath('base_unlogged')}); + +# Make sure main and init forks exist +ok(-f "$pgdata/${baseUnloggedPath}_init", 'unlogged init fork in base'); +ok(-f "$pgdata/$baseUnloggedPath", 'unlogged main fork in base'); + $node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup", '-X', 'none' ], 'pg_basebackup runs'); ok(-f "$tempdir/backup/PG_VERSION", 'backup was created'); @@ -96,6 +106,12 @@ foreach my $filename ( ok(!-f "$tempdir/backup/$filename", "$filename not copied"); } +# Unlogged relation forks other than init should not be copied +ok(-f "$tempdir/backup/${baseUnloggedPath}_init", + 'unlogged init fork in backup'); +ok(!-f "$tempdir/backup/$baseUnloggedPath", + 'unlogged main fork not in backup'); + # Make sure existing backup_label was ignored. isnt(slurp_file("$tempdir/backup/backup_label"), 'DONOTCOPY', 'existing backup_label not copied'); @@ -147,7 +163,7 @@ unlink "$pgdata/$superlongname"; # skip on Windows. SKIP: { - skip "symlinks not supported on Windows", 11 if ($windows_os); + skip "symlinks not supported on Windows", 15 if ($windows_os); # Move pg_replslot out of $pgdata and create a symlink to it. $node->stop; @@ -177,6 +193,19 @@ SKIP: my @tblspc_tars = glob "$tempdir/tarbackup2/[0-9]*.tar"; is(scalar(@tblspc_tars), 1, 'one tablespace tar was created'); + # Create an unlogged table to test that forks other than init are not copied. + $node->safe_psql('postgres', + 'CREATE UNLOGGED TABLE tblspc1_unlogged (id int) TABLESPACE tblspc1;'); + + my $tblspc1UnloggedPath = $node->safe_psql( + 'postgres', q{select pg_relation_filepath('tblspc1_unlogged')}); + + # Make sure main and init forks exist + ok(-f "$pgdata/${tblspc1UnloggedPath}_init", + 'unlogged init fork in tablespace'); + ok(-f "$pgdata/$tblspc1UnloggedPath", + 'unlogged main fork in tablespace'); + $node->command_fails( [ 'pg_basebackup', '-D', "$tempdir/backup1", '-Fp' ], 'plain format with tablespaces fails without tablespace mapping'); @@ -195,11 +224,20 @@ SKIP: "tablespace symlink was updated"); closedir $dh; + # Unlogged relation forks other than init should not be copied + my ($tblspc1UnloggedBackupPath) = $tblspc1UnloggedPath =~ /[^\/]*\/[^\/]*\/[^\/]*$/g; + + ok(-f "$tempdir/tbackup/tblspc1/${tblspc1UnloggedBackupPath}_init", + 'unlogged init fork in tablespace backup'); + ok(!-f "$tempdir/tbackup/tblspc1/$tblspc1UnloggedBackupPath", + 'unlogged main fork not in tablespace backup'); + ok( -d "$tempdir/backup1/pg_replslot", 'pg_replslot symlink copied as directory'); mkdir "$tempdir/tbl=spc2"; $node->safe_psql('postgres', "DROP TABLE test1;"); + $node->safe_psql('postgres', "DROP TABLE tblspc1_unlogged;"); $node->safe_psql('postgres', "DROP TABLESPACE tblspc1;"); $node->safe_psql('postgres', "CREATE TABLESPACE tblspc2 LOCATION '$shorter_tempdir/tbl=spc2';");