Thread: Re: BUG #16663: DROP INDEX did not free up disk space: idle connection hold file marked as deleted
Re: BUG #16663: DROP INDEX did not free up disk space: idle connection hold file marked as deleted
From
Neil Chen
Date:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hi, I have tested the feature and it worked well.
One thing that doesn't matter is that the modify here seems unnecessary, right?
> mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
> {
> char *path;
> - int ret;
> + int ret = 0;
> path = relpath(rnode, forkNum
Re: BUG #16663: DROP INDEX did not free up disk space: idle connection hold file marked as deleted
From
Pavel Borisov
Date:
One thing that doesn't matter is that the modify here seems unnecessary, right?
> mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
> {
> char *path;
> - int ret;
> + int ret = 0;
> path = relpath(rnode, forkNum
I suppose it is indeed necessary as otherwise the result of the comparison is not defined in case of 'else' block in the mdunlinkfork() :
346 else
347 {348 /* Prevent other backends' fds from holding on to the disk space */
349 do_truncate(path);
.....
356 * Delete any additional segments.357 */
358 if (ret >= 0)
Re: BUG #16663: DROP INDEX did not free up disk space: idle connection hold file marked as deleted
From
Nail Carpenter
Date:
Yes, It's my fault. You're right.
Pavel Borisov <pashkin.elfe@gmail.com> 于2020年11月19日周四 下午11:55写道：
----------^^^^^^^One thing that doesn't matter is that the modify here seems unnecessary, right?
> mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
> {
> char *path;
> - int ret;
> + int ret = 0;
> path = relpath(rnode, forkNumI suppose it is indeed necessary as otherwise the result of the comparison is not defined in case of 'else' block in the mdunlinkfork() :346 else347 {
348 /* Prevent other backends' fds from holding on to the disk space */
349 do_truncate(path);.....356 * Delete any additional segments.
357 */358 if (ret >= 0)--
So in the present logic, *ret* is always 0 if it is not in recovery mode (and other *if* conditions are not satisfied). But when the *if* condition is satisfied, it is possible to skip the deletion of additional segments.
Considering that our goal is to always try to unlink additional segments, *ret* seems unnecessary here. The code flow looks like:
> if (isRedo || .....)
> {
> int ret; /* move to here */
> ....
> }
> else
> { }
>
> /* Delete any additional segments. */
> if (true)
> ...
Or is there any reason to allow us to skip the attempt to delete additional segments in recovery mode?