Hi,
I've looked over this patch set and email thread a couple times, and I don't see anything amiss, but I'm also not terribly familiar with the subsystems this part of the code relies on. I haven't yet tried to stress test with a large database, but it seems like a good idea to do so.
I have a couple comments and questions:
0006:
+ * XXX We can optimize RelationMapOidToFileenodeForDatabase API
+ * so that instead of reading the relmap file every time, it can
+ * save it in a temporary variable and use it for subsequent
+ * calls. Then later reset it once we're done or at the
+ * transaction end.
Do we really need to consider optimizing here? Only a handful of relations will be found in the relmap, right?
+ * Once we start copying files from the source database, we need to be able
+ * to clean 'em up if we fail. Use an ENSURE block to make sure this
+ * happens. (This is not a 100% solution, because of the possibility of
+ * failure during transaction commit after we leave this routine, but it
+ * should handle most scenarios.)
This comment in master started with
- * Once we start copying subdirectories, we need to be able to clean 'em
Is the distinction important enough to change this comment? Also, is "most scenarios" still true with the patch? I haven't read into how ENSURE works.
Same with this comment change, seems fine the way it was:
- * Use an ENSURE block to make sure we remove the debris if the copy fails
- * (eg, due to out-of-disk-space). This is not a 100% solution, because
- * of the possibility of failure during transaction commit, but it should
- * handle most scenarios.
+ * Use an ENSURE block to make sure we remove the debris if the copy fails.
+ * This is not a 100% solution, because of the possibility of failure
+ * during transaction commit, but it should handle most scenarios.
And do we need additional tests? Maybe we don't, but it seems good to make sure.
I haven't looked at 0007, and I have no opinion on which approach is better.
--