db/pg_probackup
db/pg_probackup
1
0
Fork 0
mirror of https://github.com/postgrespro/pg_probackup.git synced 2025-02-13 14:58:35 +02:00
Code Issues Releases Activity

code review for issue_115. add TODO comments

Browse Source
This commit is contained in:
Anastasia 2019-09-05 16:14:50 +03:00
parent 71715d62dc
commit 0e2336fb6f
2 changed files with 31 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
src/backup.c
View File

@ -1506,6 +1506,8 @@ wait_wal_lsn(XLogRecPtr lsn, bool is_start_lsn, TimeLineID tli,
if (file_exists)
{
//TODO is this comment also relates to current segment_only case
// and should be updated?
/* Do not check LSN for previous WAL segment */
if (segment_only)
return InvalidXLogRecPtr;
@ -1805,6 +1807,11 @@ pg_stop_backup(pgBackup *backup, PGconn *pg_startbackup_conn,
XLogSegNo segno = 0;
XLogRecPtr lsn_tmp = InvalidXLogRecPtr;
/*
* TODO Let's rephrase that to be less scary for a user.
* Even though the value is invalid, it's expected postgres behaviour
* and we're trying to fix it below.
*/
elog(WARNING, "Invalid stop_backup_lsn value %X/%X",
(uint32) (stop_backup_lsn_tmp >> 32), (uint32) (stop_backup_lsn_tmp));
@ -1829,27 +1836,36 @@ pg_stop_backup(pgBackup *backup, PGconn *pg_startbackup_conn,
GetXLogSegNo(stop_backup_lsn_tmp, segno, instance_config.xlog_seg_size);
/*
* Note, that there is no guarantee that corresponding WAL file is even exists.
* Basically replica may return LSN from future and keep staying in present.
* Yeah, it sucks.
* Note, that there is no guarantee that corresponding WAL file even exists.
* Replica may return LSN from future and keep staying in present.
* Or it can return LSN with invalid XRecOff.
*
* So we should try to do the following:
* 1. Wait for current segment and look in it for the LSN >= STOP_LSN. It should
* solve the problem of occasional 0 offset on write-busy system.
* That's bad, since we want to get real LSN to save it in backup label file
* and to use it in WAL validation.
*
* So we try to do the following:
* 1. Wait for current segment and look in it for the LSN >= STOP_LSN.
* //TODO what is 'current' segment?
* //TODO how long do we wait? is there a timeout?
* It solves the problem of occasional invalid XRecOff
* on write-busy system.
* 2. Failing that, look for record in previous segment with endpoint
* equal or greater than 0 offset LSN. It may(!) solve the problem of 0 offset
* on write-idle system.
* //TODO what if if didn't?
* //TODO what kind of record that refers to?
*/
/* Wait for segment with current stop_lsn, it is ok for it to never arrive */
wait_wal_lsn(stop_backup_lsn_tmp, false, backup->tli,
false, true, WARNING, stream_wal);
/* Optimistically try to get the first record in segment with current stop_lsn */
/* Get the first record in segment with current stop_lsn */
lsn_tmp = get_first_wal_lsn(xlog_path, segno, backup->tli,
instance_config.xlog_seg_size);
/* Check if returned LSN is satisfying our requirements */
//TODO what requirements?
if (XLogRecPtrIsInvalid(lsn_tmp) ||
!XRecOffIsValid(lsn_tmp) ||
lsn_tmp < stop_backup_lsn_tmp)
@ -1877,6 +1893,9 @@ pg_stop_backup(pgBackup *backup, PGconn *pg_startbackup_conn,
stop_backup_lsn = lsn_tmp;
stop_lsn_exists = true;
}
//TODO is it good to throw an error here? Shouldn't we rather
// save it as is and mark backup as DONE instead of OK
// and check it in validation and restore?
else
elog(ERROR, "Invalid stop_backup_lsn value %X/%X",
(uint32) (stop_backup_lsn_tmp >> 32), (uint32) (stop_backup_lsn_tmp));
@ -1981,6 +2000,7 @@ pg_stop_backup(pgBackup *backup, PGconn *pg_startbackup_conn,
/*
* Wait for stop_lsn to be archived or streamed.
* //TODO is the sentence below outdated?
* If replica returned non-existent LSN, look for previous record,
* which endpoint >= stop_lsn
*/

4
src/parsexlog.c
View File

@ -581,6 +581,10 @@ get_first_wal_lsn(const char *archivedir, XLogSegNo segno,
*
* Returns LSN which points to end+1 of the last WAL record if seek_prev_segment
* is true. Otherwise returns LSN of the record prior to stop_lsn.
*
* TODO Let's think of better function name.
* it's unclear that "last" in "last_wal_lsn" refers to the
* "closest to stop_lsn backward or forward, depending on seek_prev_segment setting".
*/
XLogRecPtr
get_last_wal_lsn(const char *archivedir, XLogRecPtr start_lsn,