Re: Online checksums verification in the backend - Mailing list pgsql-hackers

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