Date: Mon, 16 Mar 2020 09:42:39 +0100
From: Julien Rouhaud <rjuju123@gmail.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Michael Paquier <michael@paquier.xyz>,
	Robert Haas <robertmhaas@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: Online checksums verification in the backend
Message-ID: <20200316084239.4hc35bjmevzztcct@nol>
References: 
 <CAOBaU_aVvMjQn=ge5qPiJOPMmOj5=ii3st5Q0Y+WuLML5sR17w@mail.gmail.com>
 <CA+TgmoavHnO20Stahs6MPe53qBqeiaCTwsktSVd7LDW+Z9pNtw@mail.gmail.com>
 <CAOBaU_aiAaas1SLS6erWa0cJStxSt+QBRPah16CqXZY4mATbTQ@mail.gmail.com>
 <20191210022633.GD72921@paquier.xyz>
 <CAOBaU_YOmFfO8Ow=EScWULku7CvUiNYdx_2QS8hfi92D=tv=OQ@mail.gmail.com>
 <20200311071823.GA4643@nol>
 <CA+fd4k5_gxSrcjXoxBk_47vB+xYGKzvcLGtwaLCbcBmW3v5oGw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+fd4k5_gxSrcjXoxBk_47vB+xYGKzvcLGtwaLCbcBmW3v5oGw@mail.gmail.com>
Precedence: bulk

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!