Hi Asim,
Thanks for having a look into the patch and for sharing your feedback.
Please find my comments inline below:
On Thu, Aug 13, 2020 at 12:36 PM Asim Praveen <pa...@vmware.com> wrote:
>
> Hi Ashutosh
>
> I stumbled upon this thread today, went through your patch and it looks good.
> A minor suggestion in sanity_check_relation():
>
> if (rel->rd_rel->relam != HEAP_TABLE_AM_OID)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("only heap AM is supported")));
>
> Instead of checking the access method OID, it seems better to check the
> handler OID like so:
>
> if (rel->rd_amhandler != HEAP_TABLE_AM_HANDLER_OID)
>
> The reason is current version of sanity_check_relation() would emit error for
> the following case even when the table structure is actually heap.
>
> create access method myam type table handler heap_tableam_handler;
> create table mytable (…) using myam;
>
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?
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
