Hi Venkata, On 11/8/16 5:47 PM, Venkata B Nagothi wrote: > Attached is the 2nd version of the patch with some enhancements.
Here's my review of the patch. 1) There are a number of whitespace error when applying the patch: $ git apply ../other/recoveryTargetIncomplete.patch-version2 ../other/recoveryTargetIncomplete.patch-version2:158: indent with spaces. else ../other/recoveryTargetIncomplete.patch-version2:160: space before tab in indent. ereport(LOG, ../other/recoveryTargetIncomplete.patch-version2:166: indent with spaces. /* ../other/recoveryTargetIncomplete.patch-version2:167: indent with spaces. * This is the position where we can choose to shutdown, pause ../other/recoveryTargetIncomplete.patch-version2:168: indent with spaces. * or promote at the end-of-the-wal if the intended recovery warning: squelched 10 whitespace errors warning: 15 lines add whitespace errors. The build did succeed after applying the patch. 2) There is no documentation. Serious changes to recovery are going to need to be documented very carefully. It will also help during review to determine you intent. 3) There are no tests. I believe that src/test/recovery/t/006_logical_decoding.pl would be the best place to insert your tests. 4) I'm not convinced that fatal errors by default are the way to go. It's a pretty big departure from what we have now. Also, I don't think it's very intuitive how recovery_target_incomplete works. For instance, if I set recovery_target_incomplete=promote and set recovery_target_time, but pick a backup after the specified time, then there will be an error "recovery_target_time %s is older..." rather than a promotion. It seems to me that there needs to be a separate setting to raise a fatal error. 5) There are really two patches here. One deals with notifying the user that replay to their recovery target is not possible (implemented here with fatal errors) and the other allows options when their recovery target appears to be possible but never arrives. As far as I can see, at this point, nobody has really signed on to this approach. I believe you will need a consensus before you can proceed with a patch this disruptive. A better approach may be to pull the first part out and raise warnings instead. This will allow you to write tests and make sure that your logic is correct without major behavioral changes to Postgres. I'm marking this as "Waiting on Author", but honestly I think "Returned with Feedback" might be more appropriate. I'll leave that up to the CFM. -- -David da...@pgmasters.net -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers