21.12.2023 15:07, Robert Haas wrote:
> On Wed, Dec 20, 2023 at 11:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
>> I've found several typos/inconsistencies introduced with 174c48050 and
>> dc2123400. Maybe you would want to fix them, while on it?:
> That's an impressively long list of mistakes in something I thought
> I'd been careful about. Sigh.
>
> I don't suppose you could provide these corrections in the form of a
> patch? I don't really want to run these sed commands across the entire
> tree and then try to figure out what's what...
Please look at the attached patch; it corrects all 29 items ("recods"
fixed in two places), but maybe you find some substitutions wrong...
I've also observed that those commits introduced new warnings:
$ CC=gcc-12 CPPFLAGS="-Wtype-limits" ./configure -q && make -s -j8
reconstruct.c: In function ‘read_bytes’:
reconstruct.c:511:24: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
511 | if (rb < 0)
| ^
reconstruct.c: In function ‘write_reconstructed_file’:
reconstruct.c:650:40: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
650 | if (rb < 0)
| ^
reconstruct.c:662:32: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
662 | if (wb < 0)
There are also two deadcode.DeadStores complaints from clang. First one is
about:
/*
* Align the wait time to prevent drift. This doesn't really matter,
* but we'd like the warnings about how long we've been waiting to say
* 10 seconds, 20 seconds, 30 seconds, 40 seconds ... without ever
* drifting to something that is not a multiple of ten.
*/
timeout_in_ms -=
TimestampDifferenceMilliseconds(current_time, initial_time) %
timeout_in_ms;
It looks like this timeout is really not used.
And the minor one (similar to many existing, maybe doesn't deserve fixing):
walsummarizer.c:808:5: warning: Value stored to 'summary_end_lsn' is never read [deadcode.DeadStores]
summary_end_lsn = private_data->read_upto;
^ ~~~~~~~~~~~~~~~~~~~~~~~
>> Also, a comment above MaybeRemoveOldWalSummaries() basically repeats a
>> comment above redo_pointer_at_last_summary_removal declaration, but
>> perhaps it should say about removing summaries instead?
> Wow, yeah. Thanks, will fix.
Thank you for paying attention to it!
Best regards,
Alexander