From: | "Bossart, Nathan" <bossartn(at)amazon(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>, Bernd Helmle <mailings(at)oopsware(dot)de>, "tsunakawa(dot)takay(at)fujitsu(dot)com" <tsunakawa(dot)takay(at)fujitsu(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: A few new options for CHECKPOINT |
Date: | 2020-12-05 05:18:54 |
Message-ID: | 4B516D8A-0891-44A3-BBAD-39A46185C15D@amazon.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for reviewing.
On 12/4/20, 5:44 PM, "Michael Paquier" <michael(at)paquier(dot)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(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.
This all seems reasonable to me. I've attached a new version of the
patch.
Nathan
Attachment | Content-Type | Size |
---|---|---|
v5-0001-Add-SPREAD-option-to-CHECKPOINT.patch | application/octet-stream | 9.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2020-12-05 06:00:56 | Re: POC: Cleaning up orphaned files using undo logs |
Previous Message | Bruce Momjian | 2020-12-05 03:52:29 | Re: Proposed patch for key managment |