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 <robertmh...@gmail.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
