From: Julien Rouhaud <rjuju123@gmail.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: Online checksums verification in the backend
Date: Mon, 16 Mar 2020 09:42:39 +0100
Message-ID: <20200316084239.4hc35bjmevzztcct@nol> (raw)
In-Reply-To: <CA+fd4k5_gxSrcjXoxBk_47vB+xYGKzvcLGtwaLCbcBmW3v5oGw@mail.gmail.com>
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>
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!
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: rjuju123@gmail.com, masahiko.sawada@2ndquadrant.com, michael@paquier.xyz, robertmhaas@gmail.com, pgsql-hackers@lists.postgresql.org, sawada.mshk@gmail.com
Subject: Re: Online checksums verification in the backend
In-Reply-To: <20200316084239.4hc35bjmevzztcct@nol>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox