Re: A few new options for CHECKPOINT

On Sat, Dec 05, 2020 at 12:11:13AM +0000, Bossart, Nathan wrote:
> On 12/4/20, 3:33 PM, "Alvaro Herrera" <alvherre(at)alvh(dot)no-ip(dot)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.
--
Michael