On Sat, Oct 28, 2017 at 4:22 AM, Chris Travers <chris.trav...@adjust.com> wrote: > The Solution: > The solution is a whitelist of directories specified which are the only ones > which are synchronised. The relevant part of this patch is: > > +/* List of directories to synchronize: > + * base data dirs (and ablespaces) > + * wal/transaction data > + * and that is it. > + * > + * This array is null-terminated to make > + * it easy to expand > + */ > > +const char *rewind_dirs[] = { > + "base", > + "global", > + "pg_commit_ts", > + "pg_logical", > + "pg_multixact", > + "pg_serial", > + "pg_subtrans", > + "pg_tblspc", > + "pg_twophase", > + "pg_wal", > + "pg_xact", > + NULL > +};
The problem with a white list, which is one reason why I do not favor it, is in the case where a feature adds in the data folder a new path for its data, particularly in the case where this is critical for a base backup. If this folder is not added in this list, then a rewind may be silently corrupted as well. There are also a set of directories in this list that we do not care about, pg_serial being one. pg_subtrans is a second, as it gets zeroed on startup. And if you think about it, pg_rewind is actually the *same* processing as a base backup taken using the replication protocol. So instead of this patch I would recommend using excludeFiles and excludeDirContents by moving this list to a common header where frontend and backend can refer to. Note that basebackup.h includes replnodes.h, so my thoughts is that you should split the current header with something like basebackup_internal.h which is backend-only, and have pg_rewind fetch the list of directories to automatically ignore as those ones. You can also simplify a bit the code of pg_rewind a bit so as things like postmaster.opts. On top of that, there is visibly no need to tweak the SQL query fetching the directory list (which is useful for debugging as shaped now actually), but just change process_source_file so as its content is not added in the file map. Then there is a second case where you do not want a set of folders to be included, but those can be useful by default, an example here being pg_wal where one might want to have an empty path to begin with. A third case is a set of folders that you do not care about, but depends on the deployment, being here "log" for example. For those you could just add an --exclude-path option which simply piles up paths into a linked list when called multiple times. This could happen with a second patch. > From there we iterate over this array for directory-based approaches in > copy_fetch.c and with one query per directory in libpq_fetch.c. This also > means shifting from the basic interface from PQexec to PQexecParams. It > would be possible to move to binary formats too, but this was not done > currently in order to simplify code review (that could be a separate > independent patch at a later time). - res = PQexec(conn, sql); + for (p = 0; rewind_dirs[p] != NULL; ++p) + { + const char *paths[1]; + paths[0] = rewind_dirs[p]; + res = PQexecParams(conn, sql, 1, NULL, paths, NULL, NULL, 0); Calling multiple times the query to list all directories is messy IMO. And this is N-costly processing if there are many files to look at, say many relation files. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers