Re: Turning recovery.conf into GUCs

Petr Jelinek <petr(at)2ndquadrant(dot)com> writes:
>
> I had a quick look, the patch does not apply cleanly anymore but it's
> just release notes so nothing too bad.

Yes, there were some ongoing changes that touched some parts of this and
I must have missed the latest one (or maybe I was waiting for it to
settle down).

> I did not do any testing yet, but I have few comments about the code:
>
> - the patch mixes functionality change with the lowercasing of the
> config variables, I wonder if those could be separated into 2 separate
> diffs - it would make review somewhat easier, but I can live with it
> as it is if it's too much work do separate into 2 patches

If we can get the lowercasing committed soon, I'd be in favor of that,
otherwise it doesn't sound as pleasing, and there's some renaming to be
made too (that standby.enabled, trigger_file, etc.)

> - the StandbyModeRequested and StandbyMode should be lowercased like
> the rest of the GUCs

Yes, except that standby_mode is linked to StandbyModeRequested, not the
other one. I can try see if there's a sane way out of this.

> - I am wondering if StandbyMode and hot_standby should be merged into
> one GUC if we are breaking backwards compatibility anyway

I also have the feeling that there's room for merging some knobs
together.

> - Could you explain the reasoning behind:
> + if ((*newval)[0] == 0)
> + xid = InvalidTransactionId;
> + else
>
> in check_recovery_target_xid() (and same check in
> check_recovery_target_timeline()), isn't the strtoul which is called
> later enough?

Well, that makes it a bit more apparent to the reader I think and we're
not abusing the fact that InvalidTransactionId equals zero.

> - The whole CopyErrorData and memory context logic is not needed in
> check_recovery_target_time() as the FlushErrorState() is not called
> there

You must be right. I recall having some trouble with strings being
free'd prematurely, but if it's ERROR, then there should be no need for
that. I'll check again.

> - The new code in StartupXLOG() like
> + if (recovery_target_xid_string != NULL)
> + InitRecoveryTarget(RECOVERY_TARGET_XID);
> +
> + if (recovery_target_time_string != NULL)
> + InitRecoveryTarget(RECOVERY_TARGET_TIME);
> +
> + if (recovery_target_name != NULL)
> + InitRecoveryTarget(RECOVERY_TARGET_NAME);
> +
> + if (recovery_target_string != NULL)
> + InitRecoveryTarget(RECOVERY_TARGET_IMMEDIATE);
>
> would probably be better in separate function that you then call
> StartupXLOG() as StartupXLOG() is crazy long already so I think it's
> preferable to not make it worse.

We can move it at top of CheckStartingAsStandby() obviously.

> - I wonder if we should rename trigger_file to standby_tigger_file

I was also suggesting that, just not sure if mixing it into the same
patch is any good.

Thank you for the review!
--
Alex