Horton

Thread

Commits

  1. Remove useless lstat() call in pg_rewind.

  1. Dubious code in pg_rewind's process_target_file()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-05T18:08:40Z

    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

  2. Re: Dubious code in pg_rewind's process_target_file()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-05T18:18:29Z

    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

  3. Re: Dubious code in pg_rewind's process_target_file()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2020-09-06T14:54:22Z

    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

  4. Re: Dubious code in pg_rewind's process_target_file()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-06T15:06:59Z

    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

  5. Re: Dubious code in pg_rewind's process_target_file()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2020-09-06T15:13:48Z

    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

  6. Re: Dubious code in pg_rewind's process_target_file()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-06T15:51:25Z

    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