Re: [HACKERS] Simplifying the interface of UpdateMinRecoveryPoint

2016-08-26 Thread Heikki Linnakangas

On 07/13/2016 04:25 PM, Michael Paquier wrote:

On Wed, Jul 13, 2016 at 8:27 PM, Amit Kapila  wrote:

On Wed, Jul 13, 2016 at 9:19 AM, Michael Paquier
 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


Re: [HACKERS] Simplifying the interface of UpdateMinRecoveryPoint

2016-07-13 Thread Michael Paquier
On Wed, Jul 13, 2016 at 10:31 PM, Simon Riggs  wrote:
> On 12 July 2016 at 23:49, Michael Paquier  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Simplifying the interface of UpdateMinRecoveryPoint

2016-07-13 Thread Simon Riggs
On 12 July 2016 at 23:49, Michael Paquier  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 Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [HACKERS] Simplifying the interface of UpdateMinRecoveryPoint

2016-07-13 Thread Michael Paquier
On Wed, Jul 13, 2016 at 8:27 PM, Amit Kapila  wrote:
> On Wed, Jul 13, 2016 at 9:19 AM, Michael Paquier
>  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Simplifying the interface of UpdateMinRecoveryPoint

2016-07-13 Thread Amit Kapila
On Wed, Jul 13, 2016 at 9:19 AM, Michael Paquier
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Simplifying the interface of UpdateMinRecoveryPoint

2016-07-12 Thread Michael Paquier
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
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e4645a3..5d69bd3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -827,7 +827,7 @@ static void RemoveXlogFile(const char *segname, XLogRecPtr PriorRedoPtr, XLogRec
 static void UpdateLastRemovedPtr(char *filename);
 static void ValidateXLOGDirectoryStructure(void);
 static void CleanupBackupHistory(void);
-static void UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force);
+static void UpdateMinRecoveryPoint(XLogRecPtr lsn);
 static XLogRecord *ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr,
 		   int emode, bool fetching_ckpt);
 static void CheckRecoveryConsistency(void);
@@ -2489,14 +2489,16 @@ XLogGetReplicationSlotMinimumLSN(void)
  * If we crash during recovery, we must reach this point again before the
  * database is consistent.
  *
- * If 'force' is true, 'lsn' argument is ignored. Otherwise, minRecoveryPoint
- * is only updated if it's not already greater than or equal to 'lsn'.
+ * If 'lsn' is InvalidXLogRecPtr, enforce its advance.  Otherwise,
+ * minRecoveryPoint is only updated if it's not already greater than or
+ * equal to 'lsn'.
  */
 static void
-UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
+UpdateMinRecoveryPoint(XLogRecPtr lsn)
 {
 	/* Quick check using our local copy of the variable */
-	if (!updateMinRecoveryPoint || (!force && lsn <= minRecoveryPoint))
+	if (!updateMinRecoveryPoint ||
+		(!XLogRecPtrIsInvalid(lsn) && lsn <= minRecoveryPoint))
 		return;
 
 	LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
@@ -2512,7 +2514,7 @@ UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
 	 */
 	if (minRecoveryPoint == 0)
 		updateMinRecoveryPoint = false;
-	else if (force || minRecoveryPoint < lsn)
+	else if (XLogRecPtrIsInvalid(lsn) || minRecoveryPoint < lsn)
 	{
 		XLogRecPtr	newMinRecoveryPoint;
 		TimeLineID	newMinRecoveryPointTLI;
@@ -2520,8 +2522,7 @@ UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
 		/*
 		 * To avoid having to update the control file too often, we update it
 		 * all the way to the last record being replayed, even though 'lsn'
-		 * would suffice for correctness.  This also allows the 'force' case
-		 * to not need a valid 'lsn' value.
+		 * would suffice for correctness.
 		 *
 		 * Another important reason for doing it this way is that the passed
 		 * 'lsn' value could be bogus, i.e., past the end of available WAL, if
@@ -2535,7 +2536,7 @@ UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
 		newMinRecoveryPointTLI = XLogCtl->replayEndTLI;
 		SpinLockRelease(>info_lck);
 
-		if (!force && newMinRecoveryPoint < lsn)
+		if (!XLogRecPtrIsInvalid(lsn) && newMinRecoveryPoint < lsn)
 			elog(WARNING,
 			   "xlog min recovery request %X/%X is past current point %X/%X",
  (uint32) (lsn >> 32), (uint32) lsn,
@@ -2582,7 +2583,8 @@ XLogFlush(XLogRecPtr record)
 	 */
 	if (!XLogInsertAllowed())
 	{
-		UpdateMinRecoveryPoint(record, false);
+		Assert(!XLogRecPtrIsInvalid(record));
+		UpdateMinRecoveryPoint(record);
 		return;
 	}
 
@@ -5244,7 +5246,7 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog)
 	/*
 	 * Update min recovery point one last time.
 	 */
-	UpdateMinRecoveryPoint(InvalidXLogRecPtr, true);
+	UpdateMinRecoveryPoint(InvalidXLogRecPtr);
 
 	/*
 	 * If the ending log segment is still open, close it (to avoid problems on
@@ -8750,7 +8752,7 @@ CreateRestartPoint(int flags)
 		(uint32) (lastCheckPoint.redo >> 32),
 		(uint32) lastCheckPoint.redo)));
 
-		UpdateMinRecoveryPoint(InvalidXLogRecPtr, true);
+		UpdateMinRecoveryPoint(InvalidXLogRecPtr);
 		if (flags & CHECKPOINT_IS_SHUTDOWN)
 		{
 			LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers