Обсуждение: small bug in recoveryStopsHere()
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
Вложения
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
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