Thread: pg_amcheck: Fix block number parsing on command line
It seems to me that when using the pg_amcheck --startblock and --endblock options on platforms where sizeof(long) == 4, you cannot specify higher block numbers (unless you do tricks with negative numbers). The attached patch should fix this by using strtoul() instead of strtol(). I also tightened up the number scanning a bit in other ways, similar to the code in other frontend utilities. I know some people have been working on tightening all this up. Please check that it's up to speed.
Attachment
> On Jul 22, 2021, at 7:56 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > Please check that it's up to speed. > <0001-pg_amcheck-Fix-block-number-parsing-on-command-line.patch> This looks correct to me. Thanks for the fix. Your use of strtoul compares favorably to that in pg_resetwal in that you are checking errno and it is not. The consequenceis: bin % ./pg_resetwal/pg_resetwal -e 1111111111111111111111111111111111111111111111111111 pg_resetwal: error: transaction ID epoch (-e) must not be -1 bin % ./pg_resetwal/pg_resetwal -e junkstring pg_resetwal: error: invalid argument for option -e Try "pg_resetwal --help" for more information. Unless people are relying on this behavior, I would think pg_resetwal should complain of an invalid argument for both ofthose, rather than complaining about -1. That's not to do with this patch, but if we're tightening up the use of strtolin frontend tools, maybe we can use the identical logic in both places. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jul 22, 2021 at 04:56:56PM +0200, Peter Eisentraut wrote: > It seems to me that when using the pg_amcheck --startblock and --endblock > options on platforms where sizeof(long) == 4, you cannot specify higher > block numbers (unless you do tricks with negative numbers). The attached > patch should fix this by using strtoul() instead of strtol(). I also > tightened up the number scanning a bit in other ways, similar to the code in > other frontend utilities. I know some people have been working on > tightening all this up. Please check that it's up to speed. Yeah, some work is happening to tighten all that. Saying that, the first round of review I did for the option parsing is not involving option types other than int32, so the block options of pg_amcheck are not changing for now, and what you are suggesting here is fine for the moment for 14~. Still, note that I am planning to change that on HEAD with an option parsing API for int64 that could be used for block numbers, eliminating for example the 4 translatable strings for the code being changed here. -- Michael
Attachment
On 22.07.21 18:18, Mark Dilger wrote: >> On Jul 22, 2021, at 7:56 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: >> >> Please check that it's up to speed. >> <0001-pg_amcheck-Fix-block-number-parsing-on-command-line.patch> > > This looks correct to me. Thanks for the fix. Committed this to 14 and master. > Your use of strtoul compares favorably to that in pg_resetwal in that you are checking errno and it is not. The consequenceis: > > bin % ./pg_resetwal/pg_resetwal -e 1111111111111111111111111111111111111111111111111111 > pg_resetwal: error: transaction ID epoch (-e) must not be -1 > bin % ./pg_resetwal/pg_resetwal -e junkstring > pg_resetwal: error: invalid argument for option -e > Try "pg_resetwal --help" for more information. > > Unless people are relying on this behavior, I would think pg_resetwal should complain of an invalid argument for both ofthose, rather than complaining about -1. That's not to do with this patch, but if we're tightening up the use of strtolin frontend tools, maybe we can use the identical logic in both places. Committed a fix for this to master.