Thread: 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 
https://www.postgresql.org/message-id/f155aab5-1323-8d0c-9e3b-32703124bf00%40iki.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
> https://www.postgresql.org/message-id/f155aab5-1323-8d0c-9e3b-32703124bf00%40iki.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
>> https://www.postgresql.org/message-id/f155aab5-1323-8d0c-9e3b-32703124bf00%40iki.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