Dubious code in pg_rewind's process_target_file()
scan-build complains that "exists = false" is a dead store,
which it is:
process_target_file(const char *path, file_type_t type, size_t oldsize,
const char *link_target)
{
bool exists;
...
if (lstat(localpath, &statbuf) < 0)
{
if (errno != ENOENT)
pg_fatal("could not stat file \"%s\": %s\n",
localpath, strerror(errno));
exists = false;
}
...
exists = (bsearch(&key_ptr, map->array, map->narray, sizeof(file_entry_t *),
path_cmp) != NULL);
It looks to me like we could replace "exists = false" with "return",
rather than uselessly constructing a FILE_ACTION_REMOVE entry for
a file we've already proven is not there. This seems to have been
copy-and-pasted from process_source_file, without much thought for
the fact that the situations are quite different.
regards, tom lane
I wrote:
It looks to me like we could replace "exists = false" with "return",
rather than uselessly constructing a FILE_ACTION_REMOVE entry for
a file we've already proven is not there.
Or actually, maybe we should just drop the lstat call altogether?
AFAICS it's 99.99% redundant with the lstat that traverse_datadir
has done nanoseconds before. Yeah, maybe somebody managed to drop
the file in between, but the FILE_ACTION_REMOVE code would have to
deal with such cases anyway in case a drop occurs later.
regards, tom lane
On 05/09/2020 21:18, Tom Lane wrote:
I wrote:
It looks to me like we could replace "exists = false" with "return",
rather than uselessly constructing a FILE_ACTION_REMOVE entry for
a file we've already proven is not there.Or actually, maybe we should just drop the lstat call altogether?
AFAICS it's 99.99% redundant with the lstat that traverse_datadir
has done nanoseconds before. Yeah, maybe somebody managed to drop
the file in between, but the FILE_ACTION_REMOVE code would have to
deal with such cases anyway in case a drop occurs later.
Agreed, the lstat() doesn't do anything interesting.
This is refactored away by the patches discussed at
/messages/by-id/f155aab5-1323-8d0c-9e3b-32703124bf00@iki.fi.
But maybe we should still clean it up in the back-branches.
- Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 05/09/2020 21:18, Tom Lane wrote:
Or actually, maybe we should just drop the lstat call altogether?
Agreed, the lstat() doesn't do anything interesting.
This is refactored away by the patches discussed at
/messages/by-id/f155aab5-1323-8d0c-9e3b-32703124bf00@iki.fi.
But maybe we should still clean it up in the back-branches.
Ah, I'd not been paying much attention to that work, but I
see you are getting rid of the lstat().
I propose to remove the lstat() in the back branches, but not touch
HEAD so as not to cause extra merge effort for your patch.
regards, tom lane
On 06/09/2020 18:06, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 05/09/2020 21:18, Tom Lane wrote:
Or actually, maybe we should just drop the lstat call altogether?
Agreed, the lstat() doesn't do anything interesting.
This is refactored away by the patches discussed at
/messages/by-id/f155aab5-1323-8d0c-9e3b-32703124bf00@iki.fi.
But maybe we should still clean it up in the back-branches.Ah, I'd not been paying much attention to that work, but I
see you are getting rid of the lstat().I propose to remove the lstat() in the back branches, but not touch
HEAD so as not to cause extra merge effort for your patch.
Thanks! Feel free to push it to HEAD, too, the merge conflict will be
trivial to resolve.
- Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 06/09/2020 18:06, Tom Lane wrote:
I propose to remove the lstat() in the back branches, but not touch
HEAD so as not to cause extra merge effort for your patch.
Thanks! Feel free to push it to HEAD, too, the merge conflict will be
trivial to resolve.
OK, done that way.
regards, tom lane