Re: [HACKERS] patch proposal

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.