Message-ID: <beb9c43d6492ffc273d236e4b93270dfad035f2f.camel@j-davis.com>
Subject: Re: Comments on Custom RMGRs
From: Jeff Davis <pgsql@j-davis.com>
To: Danil Anisimow <anisimow.d@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, PostgreSQL Hackers
	 <pgsql-hackers@postgresql.org>
Date: Thu, 21 Mar 2024 12:02:03 -0700
In-Reply-To: <CABm2Ma6avU5hkk7dd+G2oRNJZJtLOT=Yzeq6fb0v2rf7_YYp7g@mail.gmail.com>
References: 
	<CANbhV-E4pTWeF-DsdaGsOrjJNFWPaR+Dstjrnkqvf9JFFgOKKQ@mail.gmail.com>
	 <f781f8f198567cee9e3776487bab3e41a7b91b82.camel@j-davis.com>
	 <20220512034010.4oqa76pasrulkw32@alap3.anarazel.de>
	 <CANbhV-G7SfdRLR6r7sxY0uL=TRU0S=cxkWJGWyTdss3u8LFK=g@mail.gmail.com>
	 <20220512234207.pwwp6q33f72byet2@alap3.anarazel.de>
	 <CANbhV-FFgAFeNZ=qNc7Fr991Asym+sfj=R5hb02XeRE6067=hg@mail.gmail.com>
	 <CABm2Ma7v4=W-1vH=kiA4M352F=Nd-Nvd_QBZufJ6EMGsS7Q-_A@mail.gmail.com>
	 <0892cd00635c8bcd458de6d43d31cf61953da1b2.camel@j-davis.com>
	 <CABm2Ma5Hi9PU_wWP0f=YFu9aDDdeSdX2+JFDL_oDH-QHgexsgg@mail.gmail.com>
	 <727b0f3b48aec2a4f968bf11c6fa8ca6382b6cca.camel@j-davis.com>
	 <CABm2Ma6avU5hkk7dd+G2oRNJZJtLOT=Yzeq6fb0v2rf7_YYp7g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/beb9c43d6492ffc273d236e4b93270dfad035f2f.camel%40j-davis.com>
Precedence: bulk

On Thu, 2024-03-21 at 19:47 +0700, Danil Anisimow wrote:
> [pgss_001.v1.patch] adds a custom resource manager to the
> pg_stat_statements extension.

Did you consider moving the logic for loading the initial contents from
disk from pgss_shmem_startup to .rmgr_startup?

> The rm_checkpoint hook allows saving shared memory data to disk at
> each checkpoint. However, for pg_stat_statements, it matters when the
> checkpoint occurred. When the server shuts down, pgss deletes the
> temporary file of query texts. In other cases, this is unacceptable.
> To provide this capability, a flags parameter was added to the
> rm_checkpoint hook. The changes are presented in [rmgr_003.v2.patch].

Overall this seems fairly reasonable to me. I think this will work for
similar extensions, where the data being stored is independent from the
buffers.

My biggest concern is that it might not be quite right for a table AM
that has complex state that needs action to be taken at a slightly
different time, e.g. right after CheckPointBuffers().

Then again, the rmgr is a low-level API, and any extension using it
should be prepared to adapt to changes. If it works for pgss, then we
know it works for at least one thing, and we can always improve it
later. For instance, we might call the hook several times and pass it a
"phase" argument.

Regards,
	Jeff Davis