Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1redrL-00CZME-1A for pgsql-hackers@arkaria.postgresql.org; Mon, 26 Feb 2024 16:29:43 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1redrJ-00F3uo-2a for pgsql-hackers@arkaria.postgresql.org; Mon, 26 Feb 2024 16:29:41 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1redrI-00F3ug-Km for pgsql-hackers@lists.postgresql.org; Mon, 26 Feb 2024 16:29:41 +0000 Received: from mail-oi1-x231.google.com ([2607:f8b0:4864:20::231]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1redrE-001ElN-Rf for pgsql-hackers@postgresql.org; Mon, 26 Feb 2024 16:29:39 +0000 Received: by mail-oi1-x231.google.com with SMTP id 5614622812f47-3c1a172e46bso695562b6e.3 for ; Mon, 26 Feb 2024 08:29:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708964977; x=1709569777; darn=postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=RxMXBVo8ANcT9AWH3u4uaCL2/s9iKEGXArzjLR0SBiQ=; b=UFtO4YQ6xUlIY/c3D5hDQ7Caqf46fe4pr7H1JDWj7A8TxAVMtju9SlWMcKXB2W+B2U utxWhyKiHRrpLg6lCl8m/9WrAG6yuR5WDpB2gFqbUzLQFx2276mlH9dhTQxTr369zMLM PbMpOk9tpx8yjhrW/pfiU1l7M1aYj7H5MAF08r0jWmkGZeUnfJ3IPMHVwNOn1Ze+0irr +9dFAzCMjtnXWv3fzWCkHNVGP32ovJABmbBKJI8hHZSVmZ1cOoTnvdlDYZnbWa/mSScJ zBLVNBnHf8uGNa2f/WU4I/knNqEdESlcH8vivDECgkbHtmWgObOkZdbpXMEtS/wgRf1B 830A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708964977; x=1709569777; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=RxMXBVo8ANcT9AWH3u4uaCL2/s9iKEGXArzjLR0SBiQ=; b=HGkEorW1ZOVyBlRg2S7BGVQ9wyhb9QE334o4UFw6+Qmq+GN28t2dD3/MfZNlwEGVY+ tcHfdCM/gCsDVxAogQNLA6Uem6o72J42N8k8gcfTQNYMyuAOKOVxr0XEg+TxmINuBb5h PLXJGvrf5+isSUm2mFOezwmNjZa5zv34Kbh5kc2XXXjMgY9moB61hg8qKH+Hq7ZMzRj4 faglJZJOcmrBOUFTw18t59rZ+FkqW2ARneoDzuGMK2JmtD13fhB9TaxiQY29LAfWBOME PTEJxbhVzVVnLP+YMycoqASNBhSSlga39Rp7+vWV3Bci6pLDvbpMzJOzs1qxH4WF1NXn epIg== X-Forwarded-Encrypted: i=1; AJvYcCWJBibdaSh3BfZZMfLugsDiPCf6hVhxNfsXylEEFpcc+tEtoDACLbbrVRwsYyw9zm+ypteXCMDKnbVF/2M1TD8trqEi/51cOSUHkMmG X-Gm-Message-State: AOJu0YzK6OVRrWIHhF9G9HRfS3F7VoDwam1q7lEYl6388okQ5j8vGwvP H2XMLdJR0QeZV0riDOom3IH4+YWQgA+8dubkZJQ0a1MFEQ3gY0urTAGqBCFcSNtbPPGOmOhLQ3s LXZGRm63VGfQ7MI1HYte//dkhQQk= X-Google-Smtp-Source: AGHT+IFBeKzVvzdxuqN1wxBlfze8pxyH1gV8r0C0tqdR7SMglrYE+ZwPAUtpIQOX8eeCRBq2W0xH8jCndCKMB+TGkLY= X-Received: by 2002:a05:6808:d4c:b0:3c1:aa85:7ab1 with SMTP id w12-20020a0568080d4c00b003c1aa857ab1mr996526oik.50.1708964977358; Mon, 26 Feb 2024 08:29:37 -0800 (PST) MIME-Version: 1.0 References: <20220512034010.4oqa76pasrulkw32@alap3.anarazel.de> <20220512234207.pwwp6q33f72byet2@alap3.anarazel.de> In-Reply-To: From: Danil Anisimow Date: Mon, 26 Feb 2024 23:29:26 +0700 Message-ID: Subject: Re: Comments on Custom RMGRs To: Simon Riggs Cc: Andres Freund , Jeff Davis , PostgreSQL Hackers Content-Type: multipart/alternative; boundary="00000000000068bbfb06124b6a37" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --00000000000068bbfb06124b6a37 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, The checkpoint hook looks very useful, especially for extensions that have their own storage, like pg_stat_statements. For example, we can keep work data in shared memory and save it only during checkpoints. When recovering, we need to read all the data from the disk and then repeat the latest changes from the WAL. On Mon, Feb 26, 2024 at 2:42=E2=80=AFPM Simon Riggs wrote: > On Fri, 13 May 2022 at 00:42, Andres Freund wrote: > > > > On 2022-05-12 22:26:51 +0100, Simon Riggs wrote: > > > On Thu, 12 May 2022 at 04:40, Andres Freund wrote: > > > > I'm not happy with the idea of random code being executed in the middle of > > > > CheckPointGuts(), without any documentation of what is legal to do at that > > > > point. > > > > > > The "I'm not happy.." ship has already sailed with pluggable rmgrs. > > > > I don't agree. The ordering within a checkpoint is a lot more fragile than > > what you do in an individual redo routine. > > Example? > > > > > Checkpoints exist for one purpose - as the starting place for recovery. > > > > > > Why would we allow pluggable recovery without *also* allowing > > > pluggable checkpoints? > > > > Because one can do a lot of stuff with just pluggable WAL records, without > > integrating into checkpoints? > > > > Note that I'm *not* against making checkpoint extensible - I just think it > > needs a good bit of design work around when the hook is called etc. > > When was any such work done previously for any other hook?? That isn't needed. > > Checkpoints aren't complete until all data structures have > checkpointed, so there are no problems from a partial checkpoint being > written. > > As a result, the order of actions in CheckpointGuts() is mostly > independent of each other. The SLRUs are all independent of each > other, as is CheckPointBuffers(). > > The use cases I'm trying to support aren't tricksy modifications of > existing code, they are just entirely new data structures which are > completely independent of other Postgres objects. > > > > I definitely think it's too late in the cycle to add checkpoint extensibility > > now. > > > > > > > > To actually be useful we'd likely need multiple calls to such an rmgr > > > > callback, with a parameter where in CheckPointGuts() we are. Right now the > > > > sequencing is explicit in CheckPointGuts(), but with the proposed callback, > > > > that'd not be the case anymore. > > > > > > It is useful without the extra complexity you mention. > > > > Shrug. The documentation work definitely is needed. Perhaps we can get away > > without multiple callbacks within a checkpoint, I think it'll become more > > apparent when writing information about the precise point in time the > > checkpoint callback is called. > > You seem to be thinking in terms of modifying the existing actions in > CheckpointGuts(). I don't care about that. Anybody that wishes to do > that can work out the details of their actions. > > There is nothing to document, other than "don't do things that won't > work". How can anyone enumerate all the things that wouldn't work?? > > There is no list of caveats for any other hook. Why is it needed here? There are easily reproducible issues where rm_checkpoint() throws an ERROR. When it occurs at the end-of-recovery checkpoint, the server fails to start with a message like this: ERROR: Test error FATAL: checkpoint request failed HINT: Consult recent messages in the server log for details. Even if we remove the broken extension from shared_preload_libraries, we get the following message in the server log: FATAL: resource manager with ID 128 not registered HINT: Include the extension module that implements this resource manager in shared_preload_libraries. In both cases, with or without the extension in shared_preload_libraries, the server cannot start. This seems like a programmer's problem, but what should the user do after receiving such messages? Maybe it would be safer to use something like after_checkpoint_hook, which would be called after the checkpoint is completed? This is enough for some cases when we only need to save shared memory to disk. -- Regards, Daniil Anisimov Postgres Professional: http://postgrespro.com --00000000000068bbfb06124b6a37 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

The checkpoint hook looks very useful, especial= ly for extensions that have their own storage, like pg_stat_statements.
= For example, we can keep work data in shared memory and save it only during= checkpoints.
When recovering, we need to read all the data from the dis= k and then repeat the latest changes from the WAL.

On Mon, Feb 26, 2= 024 at 2:42=E2=80=AFPM Simon Riggs <simon.riggs@enterprisedb.com> wrote:
> On Fri, 13 = May 2022 at 00:42, Andres Freund <= andres@anarazel.de> wrote:
> >
> > On 2022-05-12 2= 2:26:51 +0100, Simon Riggs wrote:
> > > On Thu, 12 May 2022 at = 04:40, Andres Freund <andres@anara= zel.de> wrote:
> > > > I'm not happy with the ide= a of random code being executed in the middle of
> > > > Che= ckPointGuts(), without any documentation of what is legal to do at that
= > > > > point.
> > >
> > > The "I&= #39;m not happy.." ship has already sailed with pluggable rmgrs.
&g= t; >
> > I don't agree. The ordering within a checkpoint is= a lot more fragile than
> > what you do in an individual redo rou= tine.
>
> Example?
>
>
> > > Checkpoint= s exist for one purpose - as the starting place for recovery.
> > = >
> > > Why would we allow pluggable recovery without *also*= allowing
> > > pluggable checkpoints?
> >
> >= ; Because one can do a lot of stuff with just pluggable WAL records, withou= t
> > integrating into checkpoints?
> >
> > Note= that I'm *not* against making checkpoint extensible - I just think it<= br>> > needs a good bit of design work around when the hook is called= etc.
>
> When was any such work done previously for any other = hook?? That isn't needed.
>
> Checkpoints aren't comple= te until all data structures have
> checkpointed, so there are no pro= blems from a partial checkpoint being
> written.
>
> As a= result, the order of actions in CheckpointGuts() is mostly
> indepen= dent of each other. The SLRUs are all independent of each
> other, as= is CheckPointBuffers().
>
> The use cases I'm trying to su= pport aren't tricksy modifications of
> existing code, they are j= ust entirely new data structures which are
> completely independent o= f other Postgres objects.
>
>
> > I definitely think i= t's too late in the cycle to add checkpoint extensibility
> > = now.
> >
> >
> > > > To actually be useful= we'd likely need multiple calls to such an rmgr
> > > >= callback, with a parameter where in CheckPointGuts() we are. Right now the=
> > > > sequencing is explicit in CheckPointGuts(), but wit= h the proposed callback,
> > > > that'd not be the case = anymore.
> > >
> > > It is useful without the extra= complexity you mention.
> >
> > Shrug. The documentation= work definitely is needed. Perhaps we can get away
> > without mu= ltiple callbacks within a checkpoint, I think it'll become more
>= > apparent when writing information about the precise point in time the=
> > checkpoint callback is called.
>
> You seem to be= thinking in terms of modifying the existing actions in
> CheckpointG= uts(). I don't care about that. Anybody that wishes to do
> that = can work out the details of their actions.
>
> There is nothing= to document, other than "don't do things that won't
> w= ork". How can anyone enumerate all the things that wouldn't work??=
>
> There is no list of caveats for any other hook. Why is it = needed here?

There are easily reproducible issues where rm_checkpoin= t() throws an ERROR.
When it occurs at the end-of-recovery checkpoint, t= he server fails to start with a message like this:
ERROR: =C2=A0Test err= or
FATAL: =C2=A0checkpoint request failed
HINT: =C2=A0Consult recent = messages in the server log for details.

Even if we remove the broken= extension from shared_preload_libraries, we get the following message in t= he server log:
FATAL: =C2=A0resource manager with ID 128 not registered<= br>HINT: =C2=A0Include the extension module that implements this resource m= anager in shared_preload_libraries.

In both cases, with or without t= he extension in shared_preload_libraries, the server cannot start.

T= his seems like a programmer's problem, but what should the user do afte= r receiving such messages?

Maybe it would be safer to use something = like after_checkpoint_hook, which would be called after the checkpoint is c= ompleted?
This is enough for some cases when we only need to save shared= memory to disk.

--
Regards,
Daniil Anisimov
Postgres Profe= ssional: http://postgrespro.com
<= /div> --00000000000068bbfb06124b6a37--