Thanks for reviewing.
On 12/4/20, 5:44 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Sat, Dec 05, 2020 at 12:11:13AM +0000, Bossart, Nathan wrote:
>> On 12/4/20, 3:33 PM, "Alvaro Herrera" <alvherre@alvh.no-ip.org> wrote:
>>> Instead of adding checkpt_option_list, how about utility_option_list?
>>> It seems intended for reuse.
>
> +1. It is intended for reuse.
>
>> Ah, good call. That simplifies the grammar changes quite a bit.
>
> +CHECKPOINT;
> +CHECKPOINT (SPREAD);
> +CHECKPOINT (SPREAD FALSE);
> +CHECKPOINT (SPREAD ON);
> +CHECKPOINT (SPREAD 0);
> +CHECKPOINT (SPREAD 2);
> +ERROR: spread requires a Boolean value
> +CHECKPOINT (NONEXISTENT);
> +ERROR: unrecognized CHECKPOINT option "nonexistent"
> +LINE 1: CHECKPOINT (NONEXISTENT);
> Testing for negative cases like those two last ones is fine by me, but
> I don't like much the idea of running 5 checkpoints as part of the
> main regression test suite (think installcheck with a large shared
> buffer pool for example).
>
> --- a/src/include/postmaster/bgwriter.h
> +++ b/src/include/postmaster/bgwriter.h
> @@ -15,6 +15,8 @@
> #ifndef _BGWRITER_H
> #define _BGWRITER_H
>
> +#include "nodes/parsenodes.h"
> +#include "parser/parse_node.h"
> I don't think you need to include parsenodes.h here.
>
> +void
> +ExecCheckPointStmt(ParseState *pstate, CheckPointStmt *stmt)
> +{
> Nit: perhaps this could just be ExecCheckPoint()? See the existing
> ExecVacuum().
>
> + flags = CHECKPOINT_WAIT |
> + (RecoveryInProgress() ? 0 : CHECKPOINT_FORCE) |
> + (spread ? 0 : CHECKPOINT_IMMEDIATE);
> The handling done for CHECKPOINT_FORCE and CHECKPOINT_WAIT deserve
> a comment.
This all seems reasonable to me. I've attached a new version of the
patch.
Nathan