Thread
Commits
-
amcheck: Improve confusing message
- 953050236ab2 19 (unreleased) landed
-
confusing message in check_tuple
jian he <jian.universality@gmail.com> — 2025-06-12T06:26:29Z
hi. in contrib/amcheck/verify_heapam.c, check_tuple report_corruption(ctx, psprintf("number of attributes %u exceeds maximum expected for table %u", ctx->natts, RelationGetDescr(ctx->rel)->natts)); i think it should be report_corruption(ctx, psprintf("number of attributes %u exceeds maximum expected for table %u", ctx->natts, RelationGetRelid(ctx->rel))); or we can rephrase it another way, also mentioning ``RelationGetDescr(ctx->rel)->natts``. -
Re: confusing message in check_tuple
Peter Eisentraut <peter@eisentraut.org> — 2025-06-17T05:21:13Z
On 12.06.25 08:26, jian he wrote: > in contrib/amcheck/verify_heapam.c, check_tuple > report_corruption(ctx, > psprintf("number of attributes %u exceeds > maximum expected for table %u", > ctx->natts, > RelationGetDescr(ctx->rel)->natts)); Agreed this is misleading. > i think it should be > report_corruption(ctx, > psprintf("number of attributes %u exceeds > maximum expected for table %u", > ctx->natts, > RelationGetRelid(ctx->rel))); > > or we can rephrase it another way, also mentioning > ``RelationGetDescr(ctx->rel)->natts``. I think they did want to mention RelationGetDescr(ctx->rel)->natts. How about "number of attributes %u exceeds maximum expected for table (%u)" -
Re: confusing message in check_tuple
jian he <jian.universality@gmail.com> — 2025-06-17T08:17:48Z
On Tue, Jun 17, 2025 at 1:21 PM Peter Eisentraut <peter@eisentraut.org> wrote: > > On 12.06.25 08:26, jian he wrote: > > in contrib/amcheck/verify_heapam.c, check_tuple > > report_corruption(ctx, > > psprintf("number of attributes %u exceeds > > maximum expected for table %u", > > ctx->natts, > > RelationGetDescr(ctx->rel)->natts)); > > Agreed this is misleading. > > > i think it should be > > report_corruption(ctx, > > psprintf("number of attributes %u exceeds > > maximum expected for table %u", > > ctx->natts, > > RelationGetRelid(ctx->rel))); > > > > or we can rephrase it another way, also mentioning > > ``RelationGetDescr(ctx->rel)->natts``. > > I think they did want to mention RelationGetDescr(ctx->rel)->natts. How > about > > "number of attributes %u exceeds maximum expected for table (%u)" > ctx->natts is "int", so it should be psprintf( "number of attributes %d exceeds maximum expected for table (%u)", ctx->natts, RelationGetRelid(ctx->rel)); ? we also need change src/bin/pg_amcheck/t/004_verify_heapam.pl in there, we have push @expected, qr/${$header}number of attributes 67 exceeds maximum expected for table 3/; push @expected, qr/${$header}number of attributes 2047 exceeds maximum expected for table 3/; -
Re: confusing message in check_tuple
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-06-18T09:46:51Z
Hi, On Tue, Jun 17, 2025 at 07:21:13AM +0200, Peter Eisentraut wrote: > On 12.06.25 08:26, jian he wrote: > > in contrib/amcheck/verify_heapam.c, check_tuple > > report_corruption(ctx, > > psprintf("number of attributes %u exceeds > > maximum expected for table %u", > > ctx->natts, > > RelationGetDescr(ctx->rel)->natts)); > > Agreed this is misleading. > > > i think it should be > > report_corruption(ctx, > > psprintf("number of attributes %u exceeds > > maximum expected for table %u", > > ctx->natts, > > RelationGetRelid(ctx->rel))); > > > > or we can rephrase it another way, also mentioning > > ``RelationGetDescr(ctx->rel)->natts``. > > I think they did want to mention RelationGetDescr(ctx->rel)->natts. +1, I think that we usually want to be able to compare actual vs expected. > How > about > > "number of attributes %u exceeds maximum expected for table (%u)" I thought about adding the table name in the message but it looks like it's already there: " Expected corruption message output stdout /(?^:(?^ms:heap table "postgres\.public\.test", block 0, offset 9:\s+)number of attributes 2047 exceeds maximum expected for table 3) " So that your proposal makes sense to me. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com -
Re: confusing message in check_tuple
Peter Eisentraut <peter@eisentraut.org> — 2025-07-01T10:28:48Z
On 18.06.25 11:46, Bertrand Drouvot wrote: > Hi, > > On Tue, Jun 17, 2025 at 07:21:13AM +0200, Peter Eisentraut wrote: >> On 12.06.25 08:26, jian he wrote: >>> in contrib/amcheck/verify_heapam.c, check_tuple >>> report_corruption(ctx, >>> psprintf("number of attributes %u exceeds >>> maximum expected for table %u", >>> ctx->natts, >>> RelationGetDescr(ctx->rel)->natts)); >> >> Agreed this is misleading. >> >>> i think it should be >>> report_corruption(ctx, >>> psprintf("number of attributes %u exceeds >>> maximum expected for table %u", >>> ctx->natts, >>> RelationGetRelid(ctx->rel))); >>> >>> or we can rephrase it another way, also mentioning >>> ``RelationGetDescr(ctx->rel)->natts``. >> >> I think they did want to mention RelationGetDescr(ctx->rel)->natts. > > +1, I think that we usually want to be able to compare actual vs expected. > >> How >> about >> >> "number of attributes %u exceeds maximum expected for table (%u)" > > I thought about adding the table name in the message but it looks like it's already > there: > > " > Expected corruption message output stdout /(?^:(?^ms:heap table "postgres\.public\.test", block 0, offset 9:\s+)number of attributes 2047 exceeds maximum expected for table 3) > " > > So that your proposal makes sense to me. I have committed a fix for this.