Re: recovering from "found xmin ... from before relfrozenxid ..."

> On Aug 26, 2020, at 4:36 AM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
>
> This patch also takes care of all the other review comments from - [1].
>
> [1] - https://www.postgresql.org/message-id/CA%2Bfd4k6%2BJWq2MfQt5b7fSJ2wMvCes9TRfbDhVO_fQP9B8JJRAA%40mail.gmail.com
>
>
> --
> With Regards,
> Ashutosh Sharma
> EnterpriseDB:http://www.enterprisedb.com
> <v8-0001-Add-contrib-pg_surgery-to-perform-surgery-on-a-damag.patch>

Hi Ashutosh,

I took a look at the v8 patch, created a commitfest entry [1] because I did not find one already existent, and have the following review comments:

HeapTupleForceOption should be added to src/tools/pgindent/typedefs.list.

The tidcmp function can be removed, and ItemPointerCompare used directly by qsort as:

- qsort((void*) tids, ntids, sizeof(ItemPointerData), tidcmp);
+ qsort((void*) tids, ntids, sizeof(ItemPointerData),
+ (int (*) (const void *, const void *)) ItemPointerCompare);

sanity_check_tid_array() has two error messages:

"array must not contain nulls"
"empty tid array"

I would change the first to say "tid array must not contain nulls", as "tid" is the name of the parameter being checked. It is also more consistent with the second error message, but that doesn't matter to me so much, as I'd argue for removing the second check. I don't see why an empty array should draw an error. It seems more reasonable to just return early since there is no work to do. Consider if somebody uses a function that returns the tids for all corrupt tuples in a table, aggregates that into an array, and hands that to this function. It doesn't seem like an error for that aggregated array to have zero elements in it. I suppose you could emit a NOTICE in this case?

Upthread:
> On Aug 13, 2020, at 12:03 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> This looks like a very good suggestion to me. I will do this change in
>> the next version. Just wondering if we should be doing similar changes
>> in other contrib modules (like pgrowlocks, pageinspect and
>> pgstattuple) as well?
>
> It seems like it should be consistent, but I'm not sure the proposed
> change is really an improvement.

You have used Asim's proposed check:

if (rel->rd_amhandler != HEAP_TABLE_AM_HANDLER_OID)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("only the relation using heap_tableam_handler is supported")));

which Robert seems unenthusiastic about, but if you are going that direction, I think at least the language of the error message should be changed. I recommend something like:

if (rel->rd_amhandler != HEAP_TABLE_AM_HANDLER_OID)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("only the relation using heap_tableam_handler is supported")));
+ errmsg("\"%s\" does not use a heap access method",
+ RelationGetRelationName(rel))));

where "a heap access method" could also be written as "a heap table type access method", "a heap table compatible access method", and so forth. There doesn't seem to be enough precedent to dictate exactly how to phrase this, or perhaps I'm just not looking in the right place.

The header comment for function find_tids_one_page should state the requirement that the tids array must be sorted.

The heap_force_common function contains multiple ereport(NOTICE,...) within a critical section. I don't think that is normal practice. Can those reports be buffered until after the critical section is exited? You also have a CHECK_FOR_INTERRUPTS within the critical section.

[1] https://commitfest.postgresql.org/29/2700/
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company