Thread

1. small bug in recoveryStopsHere()

   Robert Haas <robertmhaas@gmail.com> — 2011-04-14T18:30:25Z

   I discovered while fooling around the other night that the named
   restore point patch introduced a small bug into recoveryStopsHere():
   the test at the top of the function now lets through two
   resource-manager IDs rather than one, but the remainder of the
   function tests only the record_info flag and not the
   resource-manager-id.  So the test for record_info == XLOG_XACT_COMMIT,
   for example, will also return true for an XLOG_CHECKPOINT_SHUTDOWN
   record, but the decoded commit time will be some random garbage rather
   than a commit time, because the format of the record is totally
   different.
   
   Absent objections, I'll push the attached fix.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   

2. Re: small bug in recoveryStopsHere()

   Jaime Casanova <jaime@2ndquadrant.com> — 2011-04-15T05:26:04Z

   On Thu, Apr 14, 2011 at 1:30 PM, Robert Haas <robertmhaas@gmail.com> wrote:
   > I discovered while fooling around the other night that the named
   > restore point patch introduced a small bug into recoveryStopsHere():
   > the test at the top of the function now lets through two
   > resource-manager IDs rather than one, but the remainder of the
   > function tests only the record_info flag and not the
   > resource-manager-id.  So the test for record_info == XLOG_XACT_COMMIT,
   > for example, will also return true for an XLOG_CHECKPOINT_SHUTDOWN
   > record, but the decoded commit time will be some random garbage rather
   > than a commit time, because the format of the record is totally
   > different.
   >
   
   i guess, that's why i originally used a more complicated aproach (now
   i can breath again, i didn't fully reminded why i use that)
   """
   ! 	couldStop = true;
     	if (record->xl_rmid != RM_XACT_ID)
   ! 		couldStop = false;
   ! 	/*
   ! 	 * Or when we found a named restore point
   ! 	 */
     	record_info = record->xl_info & ~XLR_INFO_MASK;
   + 	if ((record->xl_rmid == RM_XLOG_ID) && (record_info == XLOG_RESTORE_POINT))
   + 		couldStop = true;
   +
   + 	if (!couldStop)
   + 		return false;
   """
   
   but i agree that your solution is more readible, i don't see any
   problems from here
   
   -- 
   Jaime Casanova         www.2ndQuadrant.com
   Professional PostgreSQL: Soporte y capacitación de PostgreSQL
   
   

3. Re: small bug in recoveryStopsHere()

   Robert Haas <robertmhaas@gmail.com> — 2011-04-18T12:28:26Z

   On Fri, Apr 15, 2011 at 1:26 AM, Jaime Casanova <jaime@2ndquadrant.com> wrote:
   > On Thu, Apr 14, 2011 at 1:30 PM, Robert Haas <robertmhaas@gmail.com> wrote:
   >> I discovered while fooling around the other night that the named
   >> restore point patch introduced a small bug into recoveryStopsHere():
   >> the test at the top of the function now lets through two
   >> resource-manager IDs rather than one, but the remainder of the
   >> function tests only the record_info flag and not the
   >> resource-manager-id.  So the test for record_info == XLOG_XACT_COMMIT,
   >> for example, will also return true for an XLOG_CHECKPOINT_SHUTDOWN
   >> record, but the decoded commit time will be some random garbage rather
   >> than a commit time, because the format of the record is totally
   >> different.
   >>
   >
   > i guess, that's why i originally used a more complicated aproach (now
   > i can breath again, i didn't fully reminded why i use that)
   > """
   > !       couldStop = true;
   >        if (record->xl_rmid != RM_XACT_ID)
   > !               couldStop = false;
   > !       /*
   > !        * Or when we found a named restore point
   > !        */
   >        record_info = record->xl_info & ~XLR_INFO_MASK;
   > +       if ((record->xl_rmid == RM_XLOG_ID) && (record_info == XLOG_RESTORE_POINT))
   > +               couldStop = true;
   > +
   > +       if (!couldStop)
   > +               return false;
   > """
   >
   > but i agree that your solution is more readible, i don't see any
   > problems from here
   
   Thanks for the review; I've committed this.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company