On Fri, Oct 28, 2016 at 5:08 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, Oct 28, 2016 at 1:16 AM, Robert Haas <robertmh...@gmail.com> wrote: >> On Thu, Oct 27, 2016 at 9:05 AM, Michael Paquier >> <michael.paqu...@gmail.com> wrote: >>> On Thu, Oct 27, 2016 at 7:16 PM, Amit Kapila <amit.kapil...@gmail.com> >>> wrote: >>>> This can create problem if the checkpoint record spans across multiple >>>> segments, because you are updating minRecoveryPoint to start of >>>> checkpoint record. We need to update it to end+1 of checkpoint >>>> record. Please find attached patch which takes care of same. >>> >>> I gave up counting my mistakes on this thread, thanks. You should >>> update the comments of XLogCtlData for the new field >>> lastCheckPointEndPtr so as it is not used by the background writer but >>> when creating a new restart point to define the minimum recovery >>> point. >> >> I committed and back-patched this with some additional work on the >> comments, but I don't understand this remark. That comment seems like >> it should refer to the checkpointer in modern branches, but isn't that >> point independent of this patch? >
Yeah, I also think so. > * During recovery, we keep a copy of the latest checkpoint record here. > - * Used by the background writer when it wants to create a restartpoint. > + * lastCheckPointRecPtr points to start of checkpoint record and > + * lastCheckPointEndPtr points to end+1 of checkpoint record. Used by the > + * background writer when it wants to create a restartpoint. > > The patch committed introduces lastCheckPointEndPtr, which is not used > to decide if a restart point should be created or not. Only > lastCheckPointRecPtr fits with this purpose. lastCheckPointEndPtr is > used just to check if minRecoveryPoint should be pushed up or not. So > my point here would be to refine a bit this comment, in the shape of > that for example: > lastCheckPointRecPtr points to start of checkpoint record and > lastCheckPointEndPtr points to end+1 of checkpoint record. Both are > used by the checkpointer. lastCheckPointRecPtr is used when it wants > to create a restart point, and lastCheckPointEndPtr is used to > determine if the minimum recovery point should be updated or not. > lastCheckPointEndPtr is used *not* only to *determine* if the minimum recovery point, but also used to *update* it. Actually, if you see the comment is somewhat generic "Used by the background writer when it wants to create a restartpoint." and lastCheckPointEndPtr gets used when we create a restartpoint. I think there is no harm in further improving the comment, but I see nothing wrong as it is. -- 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
