On 07/13/2016 04:25 PM, Michael Paquier wrote:
On Wed, Jul 13, 2016 at 8:27 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
On Wed, Jul 13, 2016 at 9:19 AM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
Hence why not simplifying its interface and remove the force flag?
One point to note is that the signature and usage of
UpdateMinRecoveryPoint() is same as it was when it got introduced in
commit-cdd46c76. Now the only reasons that come to my mind for
introducing the force parameter was (a) it looks cleaner that way to
committer (b) they have some usecase for the same in mind (c) it got
have overlooked. Now, if it got introduced due to (c), then your
patch does the right thing by removing it. Personally, I feel
overloading the parameter for multiple purposes makes code less
maintainable, so retaining as it is in HEAD has some merits.
There is no way to tell what that is, but perhaps Heikki recalls
something on the matter. I am just adding him in CC.
No, I don't remember. Maybe the function originally used the
caller-supplied 'lsn' value as the value to force-update
minRecoveryPoint to. Or I anticipated that some callers might want to do
that in the future.
If we were to do this, it might be better to still have a 'force'
variable inside the function, to keep the if()s slighltly more readable,
like:
bool force = XLogRecPtrIsInvalid(lsn);
But even then, I don't think this makes it really any more readable
overall. Not worse either, but it's a wash. I'll just mark this as
rejected in the commitfest, let's move on.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
