Re: Online checksums verification in the backend

On Mon, Mar 16, 2020 at 01:53:35PM +0900, Masahiko Sawada wrote:
>
> In addition to comments from Michael-san, here are my comments:
>
> 1.
> + 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")));

Good point! I'll fix it.

> +
> + if (!DataChecksumsEnabled())
> + elog(ERROR, "Data checksums are not enabled");
>
> I think it's better to reverse the order of the above checks.

Indeed.

>
> 2.
> +#define CRF_COLS 5 /* Number of output arguments in the SRF */
>
> Should it be SRF_COLS?

Oops, will fix.

>
> 3.
> +static void
> +check_delay_point(void)
> +{
> + /* Always check for interrupts */
> + CHECK_FOR_INTERRUPTS();
> +
> + /* Nap if appropriate */
> + if (!InterruptPending && VacuumCostBalance >= VacuumCostLimit)
> + {
> + int msec;
> +
> + msec = VacuumCostDelay * VacuumCostBalance / VacuumCostLimit;
> + if (msec > VacuumCostDelay * 4)
> + msec = VacuumCostDelay * 4;
> +
> + pg_usleep(msec * 1000L);
> +
> + VacuumCostBalance = 0;
> +
> + /* Might have gotten an interrupt while sleeping */
> + CHECK_FOR_INTERRUPTS();
> + }
> +}
>
> Even if we use vacuum delay for this function, I think we need to set
> VacuumDelayActive and return if it's false, or it's better to just
> return if VacuumCostDelay == 0.

Good point, I'll fix that.

>
> 4.
> +static void
> +check_all_relations(TupleDesc tupdesc, Tuplestorestate *tupstore,
> + ForkNumber forknum)
>
> I also agree with Michael-san to remove this function. Instead we can
> check all relations by:
>
> select pg_check_relation(oid) from pg_class;

Sure, but ideally we should do that in a client program (eg. pg_checksums)
that wouldn't maintain a transaction active for the whole execution.

> 6.
> Other typos
>
> s/dirted/dirtied/
> s/explictly/explicitly/

Will fix, thanks!