Chris, * Chris Travers (chris.trav...@adjust.com) wrote: > Attached is the patch as submitted for commitfest.
This has been stuck in Waiting for Author since before the commitfest started. I'll try to kick-start it but it seems like it's stuck because there's a fundamental disagreement about if we should be using include or exclude for pg_rewind (and, possibly, pg_basebackup, et al). > Please note, I am not adverse to adding an additional --Include-path > directive if that avoids backwards-compatibility problems. However the > patch is complex enough I would really prefer review on the rest of it to > start first. This doesn't strike me as perfectly trivial and I think it is > easier to review pieces separately. I have already started on the > --Include-path directive and could probably get it attached to a later > version of the patch very soon. > > I would also like to address a couple of important points here: > > 1. I think default restrictions plus additional paths is the best, safest > way forward. Excluding shell-globs doesn't solve the "I need this > particular config file" very well particularly if we want to support this > outside of an internal environment. Shell globs also tend to be overly > inclusive and so if you exclude based on them, you run into a chance that > your rewind is corrupt for being overly exclusive. There's been a number of strong points made as to why pg_basebackup uses an exclude list but you seem to keep coming back to a concern around shell globs when considering exclusion lists. Having an exclude list doesn't mean that shell globs have to be used to in the exclude list and, indeed, there are no such globs used in the exclude list for pg_basebackup today- every single file or directory excluded by pg_basebackup is an explicit file or directory (compared using a full strcmp()). Further, the specific concerns raised are about things which are very clearly able to be dealt with using specific paths (postgresql.auto.conf, pg_log) and which an administrator is likely to be familiar with (such as pg_log, in particular). Personally, I'm a big advocate of *not* having PG's logs in the data directory and part of it is because, really, the data directory is PG's purview while logs are for the administrator. I wasn't ever a fan of postgresql.auto.conf and I'm not surprised that we're having this discussion about if it should be pulled over by pg_rewind or not. > 3. I think it would be a mistake to tie backup solutions in non-replicated > environments to replication use cases, and vice versa. Things like > replication slots (used for streaming backups) have different > considerations in different environments. Rather than build the same > infrastructure first, I think it is better to support different use cases > well and then build common infrastructure to support the different cases. > I am not against building common infrastructure for pg_rewind and > pg_basebackup. I am very much against having the core guarantees being the > exact same. Backup solutions are already involved in understanding of replicated environments as they can be used to back up from replicas rather than the primary (or perhaps using both in some cases), so I'm not really sure why you're argueing that backups are somehow different between non-replicated and replicated environments. As it relates to the question about if the core guarantees between pg_basebackup and pg_rewind being the same or not, I've not see much argument for why they'd be different. The intent of pg_rewind is to do what pg_basebackup would do, but in a more efficient manner, as discussed in the documentation for pg_rewind. I would think the next step here, as Michael suggested very early on in this thread, would be to bring the exclude list and perhaps logic for pg_basebackup into the common code and have pg_rewind leverage that instead of having its own code that largely does the same and then adding an option to exclude additional items to that. There's no sense having pg_rewind operate on files that are going to end up getting wiped out when recovery starts anyway. Perhaps there's a use-case for overriding the exclude list with a 'include' option too, but I'm not convinced there is. Thanks! Stephen
signature.asc
Description: PGP signature
