Re: pg_amcheck: Fix block number parsing on command line
On 22.07.21 18:18, Mark Dilger wrote: On Jul 22, 2021, at 7:56 AM, Peter Eisentraut 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 consequence is: bin % ./pg_resetwal/pg_resetwal -e 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 of those, rather than complaining about -1. That's not to do with this patch, but if we're tightening up the use of strtol in frontend tools, maybe we can use the identical logic in both places. Committed a fix for this to master.
Re: pg_amcheck: Fix block number parsing on command line
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 signature.asc Description: PGP signature
Re: pg_amcheck: Fix block number parsing on command line
> On Jul 22, 2021, at 7:56 AM, Peter Eisentraut > 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 consequence is: bin % ./pg_resetwal/pg_resetwal -e 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 of those, rather than complaining about -1. That's not to do with this patch, but if we're tightening up the use of strtol in frontend tools, maybe we can use the identical logic in both places. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
