confusing message in check_tuple

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)"

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/;

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

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.