Обсуждение: Temporary WAL segments files not cleaned up after an instance crash
Hi all, While playing with a standby as follows I noticed that xlogtemp.* generated in pg_wal may stay around when entering crash recovery. The test I was conducting is pretty simple: - Use a primary and a standby. - Run pgbench on the primary. - Then restart the standby with -m immediate and force WAL segment switch on the primary in a loop. Depending on the timing, one can see that those xlogtemp files stay around. Those files are here when creating a new segment from scratch and append the PID of the process creating them. Any previous file existing with the same name is unlinked. The problem is that if an instance is not really stable for a reason or another and starts crash recovery periodically, then there is a risk of accumulating those temporary files. If pg_wal is on its own partition, tuned by max_wal_size, then there is a risk to run into ENOSPC and take PostgreSQL down as new WAL segments cannot be created. Shouldn't those files be cleaned up at the beginning of crash recovery? Attached is a proposal of patch doing so. Thoughts? -- Michael
Вложения
On Mon, 14 May 2018 14:49:55 +0900 Michael Paquier <michael@paquier.xyz> wrote: > Hi all, > > While playing with a standby as follows I noticed that xlogtemp.* > generated in pg_wal may stay around when entering crash recovery. The > test I was conducting is pretty simple: > - Use a primary and a standby. > - Run pgbench on the primary. > - Then restart the standby with -m immediate and force WAL segment > switch on the primary in a loop. Depending on the timing, one can see > that those xlogtemp files stay around. Those files are here when > creating a new segment from scratch and append the PID of the process > creating them. Any previous file existing with the same name is > unlinked. > > The problem is that if an instance is not really stable for a reason or > another and starts crash recovery periodically, then there is a risk of > accumulating those temporary files. If pg_wal is on its own partition, > tuned by max_wal_size, then there is a risk to run into ENOSPC and take > PostgreSQL down as new WAL segments cannot be created. > > Shouldn't those files be cleaned up at the beginning of crash recovery? > Attached is a proposal of patch doing so. I think it makes sense to remove unnecessary temporary WAL files although I'm not sure how high the risk of ENOSPC is. The code looks fine, the patch can be applied to HEAD, and I can build this successfully. I confirmed that all tempxlog.* files are removed when restarting postgres that was shutdown immediately. One little thing I noticed is the function name "RemoveXLogTempFiles". Other similar functions are named as RemoveOldXlogFiles or RemoveXlogFile (using Xlog not XLog), so it seem to me more consistent to rename this "RemoveXlogTempFiles" or "RemoveTempXlogFiles" and so on. Regards -- Yugo Nagata <nagata@sraoss.co.jp>
On Thu, Jul 12, 2018 at 03:35:53PM +0900, Yugo Nagata wrote: > I think it makes sense to remove unnecessary temporary WAL files although > I'm not sure how high the risk of ENOSPC is. It depends on how close to the partition size limit max_wal_size is set, and how much a system is unstable. Switching on/off a VM where Postgres is located can participate in that, as well as VM snapshots taken without memory (I work a lot on those as you can guess :D). Setting it to 70% of the partition size is what I imagine is the base, but I can imagine as well people setting it at 90% or more. Still the probability is low, which is why I think that it would make sense to just fix the problem on HEAD and move on. > One little thing I noticed is the function name "RemoveXLogTempFiles". > Other similar functions are named as RemoveOldXlogFiles or RemoveXlogFile > (using Xlog not XLog), so it seem to me more consistent to rename this > "RemoveXlogTempFiles" or "RemoveTempXlogFiles" and so on. I see, a lower-case for Xlog instead of XLog. That makes sense. I have used your second suggestion in the attached. I have also changed the thing so as the format of the comment block is better even after indenting. Thoughts? -- Michael
Вложения
On 12/07/18 10:44, Michael Paquier wrote: > On Thu, Jul 12, 2018 at 03:35:53PM +0900, Yugo Nagata wrote: >> One little thing I noticed is the function name "RemoveXLogTempFiles". >> Other similar functions are named as RemoveOldXlogFiles or RemoveXlogFile >> (using Xlog not XLog), so it seem to me more consistent to rename this >> "RemoveXlogTempFiles" or "RemoveTempXlogFiles" and so on. > > I see, a lower-case for Xlog instead of XLog. That makes sense. I have > used your second suggestion in the attached. I have also changed the > thing so as the format of the comment block is better even after > indenting. Looks good to me. Just one little nitpick: > + snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlde->d_name); > + elog(DEBUG2, "removed temporary WAL file \"%s\"", path); > + unlink(path); The elog message says "removed", but the removal actually happens after the elog. "removing" would be more accurate. - Heikki
On Thu, Jul 12, 2018 at 01:15:03PM +0300, Heikki Linnakangas wrote: > On 12/07/18 10:44, Michael Paquier wrote: > > + snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlde->d_name); > > + elog(DEBUG2, "removed temporary WAL file \"%s\"", path); > > + unlink(path); > > The elog message says "removed", but the removal actually happens after the > elog. "removing" would be more accurate. Or just move the elog() after the file is actually removed? Would you be fine with that? -- Michael
Вложения
On 12/07/18 15:38, Michael Paquier wrote: > On Thu, Jul 12, 2018 at 01:15:03PM +0300, Heikki Linnakangas wrote: >> On 12/07/18 10:44, Michael Paquier wrote: >>> + snprintf(path, MAXPGPATH, XLOGDIR "/%s", xlde->d_name); >>> + elog(DEBUG2, "removed temporary WAL file \"%s\"", path); >>> + unlink(path); >> >> The elog message says "removed", but the removal actually happens after the >> elog. "removing" would be more accurate. > > Or just move the elog() after the file is actually removed? Would you > be fine with that? Sure. - Heikki
On Thu, Jul 12, 2018 at 03:40:43PM +0300, Heikki Linnakangas wrote: > Sure. Thanks for the reviews, I have pushed the patch after moving the elog() call and changing the logs to mention "WAL segments" instead of "WAL files". -- Michael
Вложения
On Thu, 12 Jul 2018 16:44:45 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Jul 12, 2018 at 03:35:53PM +0900, Yugo Nagata wrote: > > I think it makes sense to remove unnecessary temporary WAL files although > > I'm not sure how high the risk of ENOSPC is. > > It depends on how close to the partition size limit max_wal_size is set, > and how much a system is unstable. Switching on/off a VM where Postgres > is located can participate in that, as well as VM snapshots taken > without memory (I work a lot on those as you can guess :D). Setting it > to 70% of the partition size is what I imagine is the base, but I can > imagine as well people setting it at 90% or more. Thank you for your explaining this. I have understood the problem you concern well. Thanks, -- Yugo Nagata <nagata@sraoss.co.jp>