Re: new heapcheck contrib module

> On Aug 24, 2020, at 2:48 AM, Amul Sul <sulamul@gmail.com> wrote:
> 
> Few comments for v14 version:
> 
> v14-0001:
> 
> verify_heapam.c: In function ‘verify_heapam’:
> verify_heapam.c:339:14: warning: variable ‘ph’ set but not used
> [-Wunused-but-set-variable]
>   PageHeader ph;
>              ^
> verify_heapam.c: In function ‘check_toast_tuple’:
> verify_heapam.c:877:8: warning: variable ‘chunkdata’ set but not used
> [-Wunused-but-set-variable]
>  char    *chunkdata;
> 
> Got these compilation warnings

Removed.

> 
> 
> +++ b/contrib/amcheck/amcheck.h
> @@ -0,0 +1,5 @@
> +#include "postgres.h"
> +
> +Datum verify_heapam(PG_FUNCTION_ARGS);
> +Datum bt_index_check(PG_FUNCTION_ARGS);
> +Datum bt_index_parent_check(PG_FUNCTION_ARGS);
> 
> bt_index_* are needed?

This entire header file is not needed.  Removed.

> #include "access/htup_details.h"
> #include "access/xact.h"
> #include "catalog/pg_type.h"
> #include "catalog/storage_xlog.h"
> #include "storage/smgr.h"
> #include "utils/lsyscache.h"
> #include "utils/rel.h"
> #include "utils/snapmgr.h"
> #include "utils/syscache.h"
> 
> These header file inclusion to verify_heapam.c. can be omitted. Some of those
> might be implicitly got added by other header files or no longer need due to
> recent changes.

Removed.


> + *   on_error_stop:
> + *     Whether to stop at the end of the first page for which errors are
> + *     detected.  Note that multiple rows may be returned.
> + *
> + *   check_toast:
> + *     Whether to check each toasted attribute against the toast table to
> + *     verify that it can be found there.
> + *
> + *   skip:
> + *     What kinds of pages in the heap relation should be skipped.  Valid
> + *     options are "all-visible", "all-frozen", and "none".
> 
> I think it would be good if the description also includes what will be default
> value otherwise.

The defaults are defined in amcheck--1.2--1.3.sql, and I was concerned that documenting them in verify_heapam.c would create a hazard of the defaults and their documented values getting out of sync.  The handling of null arguments in verify_heapam.c was, however, duplicating the defaults from the .sql file, so I've changed that to just ereport error on null.  (I can't make the whole function strict, as some other arguments are allowed to be null.)  I have not documented the defaults in either file, as they are quite self-evident in the .sql file.  I've updated some tests that were passing null to get the default behavior to now either pass nothing or explicitly pass the argument they want.

> 
> + /*
> + * Optionally open the toast relation, if any, also protected from
> + * concurrent vacuums.
> + */
> 
> Now lock is changed to AccessShareLock, I think we need to rephrase this comment
> as well since we are not really doing anything extra explicitly to protect from
> the concurrent vacuum.

Right.  Comment changed.

> +/*
> + * Return wehter a multitransaction ID is in the cached valid range.
> + */
> 
> Typo: s/wehter/whether

Changed.

> v14-0002:
> 
> +#define NOPAGER 0
> 
> Unused macro.

Removed.

> + appendPQExpBuffer(querybuf,
> +   "SELECT c.relname, v.blkno, v.offnum, v.attnum, v.msg"
> +   "\nFROM public.verify_heapam("
> + "\nrelation := %u,"
> + "\non_error_stop := %s,"
> + "\nskip := %s,"
> + "\ncheck_toast := %s,"
> + "\nstartblock := %s,"
> + "\nendblock := %s) v, "
> + "\npg_catalog.pg_class c"
> +   "\nWHERE c.oid = %u",
> +   tbloid, stop, skip, toast, startblock, endblock, tbloid);
> [....]
> + appendPQExpBuffer(querybuf,
> +   "SELECT public.bt_index_parent_check('%s'::regclass, %s, %s)",
> +   idxoid,
> +   settings.heapallindexed ? "true" : "false",
> +   settings.rootdescend ? "true" : "false");
> 
> The assumption that the amcheck extension will be always installed in the public
> schema doesn't seem to be correct. This will not work if amcheck install
> somewhere else.

Right.  I removed the schema qualification, leaving it up to the search path.  

Thanks for the review!