Обсуждение: Simplifying the interface of UpdateMinRecoveryPoint
Hi all, As of now UpdateMinRecoveryPoint() is using two arguments: - lsn, to check if the minimum recovery point should be updated to that - force, a boolean flag to decide if the update should be enforced or not. However those two arguments are correlated. If lsn is InvalidXlogRecPtr, the minimum recovery point update will be enforced. Hence why not simplifying its interface and remove the force flag? See attached. Thanks, -- Michael
Вложения
On Wed, Jul 13, 2016 at 9:19 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > Hi all, > > As of now UpdateMinRecoveryPoint() is using two arguments: > - lsn, to check if the minimum recovery point should be updated to that > - force, a boolean flag to decide if the update should be enforced or not. > However those two arguments are correlated. If lsn is > InvalidXlogRecPtr, the minimum recovery point update will be enforced. Right. > 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. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Jul 13, 2016 at 8:27 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Wed, Jul 13, 2016 at 9:19 AM, Michael Paquier > <michael.paquier@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. -- Michael
On 12 July 2016 at 23:49, Michael Paquier <michael.paquier@gmail.com> wrote:
--
Hence why not simplifying its interface and remove the force flag?
Is this change needed as part of a feature? If not, I see no reason for change.
If we all work towards meaningful features the code can be cleaned up as we go.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jul 13, 2016 at 10:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 12 July 2016 at 23:49, Michael Paquier <michael.paquier@gmail.com> wrote: >> Hence why not simplifying its interface and remove the force flag? > > Is this change needed as part of a feature? If not, I see no reason for > change. > > If we all work towards meaningful features the code can be cleaned up as we > go. That's just refactoring. The interactions between the two arguments of this routine is plain. -- Michael
On 07/13/2016 04:25 PM, Michael Paquier wrote: > On Wed, Jul 13, 2016 at 8:27 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Wed, Jul 13, 2016 at 9:19 AM, Michael Paquier >> <michael.paquier@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