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

Hi Mark,

Thanks for the review. Please find my comments inline below:

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

This has been fixed in the v9 patch.

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

Will have a look into this.

>
> 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?
>

This comment is no more valid as per the changes done in the v9 patch.

>
> 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.
>

Same here. This also looks invalid as per the changes done in v9 patch.

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

Okay, will add a comment for this.

>
> 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.
>

This has been fixed in the v9 patch.

> [1] https://commitfest.postgresql.org/29/2700/
> —

Thanks for adding a commitfest entry for this.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com