pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedDubious code in pg_rewind's process_target_file()
6+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2020-09-06 666e9a90f0 e7f06ea60a 1f8c163c5a a7bcf391f3 75e44b471c be6667081e 7d60e41830 Remove useless lstat() call in pg_rewind.
* Dubious code in pg_rewind's process_target_file()
@ 2020-09-05 18:08 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2020-09-05 18:08 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dubious code in pg_rewind's process_target_file()
@ 2020-09-05 18:18 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2020-09-05 18:18 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dubious code in pg_rewind's process_target_file()
@ 2020-09-06 14:54 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Heikki Linnakangas @ 2020-09-06 14:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dubious code in pg_rewind's process_target_file()
@ 2020-09-06 15:06 Tom Lane <tgl@sss.pgh.pa.us>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2020-09-06 15:06 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dubious code in pg_rewind's process_target_file()
@ 2020-09-06 15:13 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Heikki Linnakangas @ 2020-09-06 15:13 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dubious code in pg_rewind's process_target_file()
@ 2020-09-06 15:51 Tom Lane <tgl@sss.pgh.pa.us>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 0 replies; 6+ messages in thread
From: Tom Lane @ 2020-09-06 15:51 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2020-09-06 15:51 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-05 18:08 Dubious code in pg_rewind's process_target_file() Tom Lane <tgl@sss.pgh.pa.us>
2020-09-05 18:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-09-06 14:54 ` Heikki Linnakangas <hlinnaka@iki.fi>
2020-09-06 15:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-09-06 15:13 ` Heikki Linnakangas <hlinnaka@iki.fi>
2020-09-06 15:51 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox