Re: Online checksums verification in the backend

On Sun, Apr 05, 2020 at 01:13:30PM +0900, Masahiko Sawada wrote:
>
> Thank you for updating the patch! The patch looks good to me. Here are
> some random comments mostly about cosmetic changes.
>

Thanks a lot for the review!

>
> 1.
> I think we can have two separate SQL functions:
> pg_check_relation(regclass, text) and pg_check_relation(regclass),
> instead of setting NULL by default to the second argument.
>

I'm fine with it, so implemented this way with the required documentation
changes.

>
> 2.
> + * Check data sanity for a specific block in the given fork of the given
> + * relation, always retrieved locally with smgrred even if a version exists in
>
> s/smgrred/smgrread/

Fixed.

>
> 3.
> + /* The buffer will have to check checked. */
> + Assert(checkit);
>
> Should it be "The buffer will have to be checked"?
>

Oops indeed, fixed.

>
> 4.
> + if (!is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES))
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("only superuser or a member of the
> pg_read_server_files role may use this function")));
>
> Looking at the definition of pg_stat_read_server_files role, this role
> seems to be for operations that could read non-database files such as
> csv files. Therefore, currently this role is used by file_fdw and COPY
> command. I personally think pg_stat_scan_tables would be more
> appropriate for this function but I'm not sure.
>

That's a very good point, especially since the documentation of this default
role is quite relevant for those functions:

"Execute monitoring functions that may take ACCESS SHARE locks on tables,
potentially for a long time."

So changed!

>
> 5.
> + /* Set cost-based vacuum delay */
> + ChecksumCostActive = (ChecksumCostDelay > 0);
> + ChecksumCostBalance = 0;
>
> s/vacuum/checksum verification/
>

Fixed.

>
> 6.
> + ereport(WARNING,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("invalid page in block %u of relation %s",
> + blkno,
> + relpath(relation->rd_smgr->smgr_rnode, forknum))));
>
> I think it's better to show the relation name instead of the relation path here.
>

I'm here using the same pattern as what ReadBuffer_common() would display if a
corrupted block is read. I think it's better to keep the format for both, so
any existing log analyzer will keep working with those new functions.

>
> 7.
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("relation \"%s\" does not have storage to be checked",
> + quote_qualified_identifier(
> + get_namespace_name(get_rel_namespace(relid)),
> + get_rel_name(relid)))));
>
> Looking at other similar error messages we don't show qualified
> relation name but the relation name gotten by
> RelationGetRelationName(relation). Can we do that here as well for
> consistency?
>

Indeed, fixed.

>
> 8.
> + if (!(rsinfo->allowedModes & SFRM_Materialize))
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("materialize mode required, but it is not " \
> + "allowed in this context")));
>
> I think it's better to have this error message in one line for easy grepping.

Fixed.

I also fixed missing leading tab in the perl TAP tests