public inbox for [email protected]
help / color / mirror / Atom feedFrom: Simon Riggs <[email protected]>
To: Andres Freund <[email protected]>
Cc: Jeff Davis <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Comments on Custom RMGRs
Date: Thu, 12 May 2022 22:26:51 +0100
Message-ID: <CANbhV-G7SfdRLR6r7sxY0uL=TRU0S=cxkWJGWyTdss3u8LFK=g@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CANbhV-E4pTWeF-DsdaGsOrjJNFWPaR+Dstjrnkqvf9JFFgOKKQ@mail.gmail.com>
<[email protected]>
<[email protected]>
On Thu, 12 May 2022 at 04:40, Andres Freund <[email protected]> wrote:
>
> On 2022-05-11 09:39:48 -0700, Jeff Davis wrote:
> > On Wed, 2022-05-11 at 15:24 +0100, Simon Riggs wrote:
> > > [PATCH: rmgr_001.v1.patch]
> > >
> > > [PATCH: rmgr_002.v1.patch]
> >
> > Thank you. Both of these look like good ideas, and I will commit them
> > in a few days assuming that nobody else sees a problem.
>
> What exactly is the use case here? Without passing in information about
> whether recovery will be performed etc, it's not at all clear how callbacks
> could something useful?
Sure, happy to do it that way.
[PATCH: rmgr_002.v2.patch]
> I don't think we should allocate a bunch of memory contexts to just free them
> immediately after?
Didn't seem a problem, but I've added code to use the flag requested above.
> > > It occurs to me that any use of WAL presumes that Checkpoints exist
> > > and do something useful. However, the custom rmgr interface doesn't
> > > allow you to specify any actions on checkpoint, so ends up being
> > > limited in scope. So I think we also need an rm_checkpoint() call -
> > > which would be a no-op for existing rmgrs.
> > > [PATCH: rmgr_003.v1.patch]
> >
> > I also like this idea, but can you describe the intended use case? I
> > looked through CheckPointGuts() and I'm not sure what else a custom AM
> > might want to do. Maybe sync special files in a way that's not handled
> > with RegisterSyncRequest()?
>
> 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.
Checkpoints exist for one purpose - as the starting place for recovery.
Why would we allow pluggable recovery without *also* allowing
pluggable checkpoints?
>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.
I see multiple uses for the rm_checkpoint() point proposed and I've
been asked multiple times for a checkpoint hook. Any rmgr that
services crash recovery for a non-smgr based storage system would need
this because the current checkpoint code only handles flushing to disk
for smgr-based approaches. That is orthogonal to other code during
checkpoint, so it stands alone quite well.
--
Simon Riggs http://www.EnterpriseDB.com/
Attachments:
[application/octet-stream] rmgr_002.v2.patch (8.0K, 2-rmgr_002.v2.patch)
download | inline diff:
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index 87e8366642..53d6222775 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -772,16 +772,20 @@ gin_redo(XLogReaderState *record)
}
void
-gin_xlog_startup(void)
+gin_xlog_startup(bool in_redo)
{
+ if (!in_redo)
+ return;
opCtx = AllocSetContextCreate(CurrentMemoryContext,
"GIN recovery temporary context",
ALLOCSET_DEFAULT_SIZES);
}
void
-gin_xlog_cleanup(void)
+gin_xlog_cleanup(bool in_redo)
{
+ if (!in_redo)
+ return;
MemoryContextDelete(opCtx);
opCtx = NULL;
}
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index df70f906b4..49681109f2 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -440,14 +440,18 @@ gist_redo(XLogReaderState *record)
}
void
-gist_xlog_startup(void)
+gist_xlog_startup(bool in_redo)
{
+ if (!in_redo)
+ return;
opCtx = createTempGistContext();
}
void
-gist_xlog_cleanup(void)
+gist_xlog_cleanup(bool in_redo)
{
+ if (!in_redo)
+ return;
MemoryContextDelete(opCtx);
}
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index f9186ca233..a0c5c6c4fa 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -1069,16 +1069,20 @@ btree_redo(XLogReaderState *record)
}
void
-btree_xlog_startup(void)
+btree_xlog_startup(bool in_redo)
{
+ if (!in_redo)
+ return;
opCtx = AllocSetContextCreate(CurrentMemoryContext,
"Btree recovery temporary context",
ALLOCSET_DEFAULT_SIZES);
}
void
-btree_xlog_cleanup(void)
+btree_xlog_cleanup(bool in_redo)
{
+ if (!in_redo)
+ return;
MemoryContextDelete(opCtx);
opCtx = NULL;
}
diff --git a/src/backend/access/spgist/spgxlog.c b/src/backend/access/spgist/spgxlog.c
index b500b2cced..3e4e6ec939 100644
--- a/src/backend/access/spgist/spgxlog.c
+++ b/src/backend/access/spgist/spgxlog.c
@@ -977,16 +977,20 @@ spg_redo(XLogReaderState *record)
}
void
-spg_xlog_startup(void)
+spg_xlog_startup(bool in_redo)
{
+ if (!in_redo)
+ return;
opCtx = AllocSetContextCreate(CurrentMemoryContext,
"SP-GiST temporary context",
ALLOCSET_DEFAULT_SIZES);
}
void
-spg_xlog_cleanup(void)
+spg_xlog_cleanup(bool in_redo)
{
+ if (!in_redo)
+ return;
MemoryContextDelete(opCtx);
opCtx = NULL;
}
diff --git a/src/backend/access/transam/rmgr.c b/src/backend/access/transam/rmgr.c
index e1d6ebbd3d..ddd9e04fdb 100644
--- a/src/backend/access/transam/rmgr.c
+++ b/src/backend/access/transam/rmgr.c
@@ -46,7 +46,7 @@ RmgrData RmgrTable[RM_MAX_ID + 1] = {
* Start up all resource managers.
*/
void
-RmgrStartup(void)
+RmgrStartup(bool in_redo)
{
for (int rmid = 0; rmid <= RM_MAX_ID; rmid++)
{
@@ -54,7 +54,7 @@ RmgrStartup(void)
continue;
if (RmgrTable[rmid].rm_startup != NULL)
- RmgrTable[rmid].rm_startup();
+ RmgrTable[rmid].rm_startup(in_redo);
}
}
@@ -62,7 +62,7 @@ RmgrStartup(void)
* Clean up all resource managers.
*/
void
-RmgrCleanup(void)
+RmgrCleanup(bool in_redo)
{
for (int rmid = 0; rmid <= RM_MAX_ID; rmid++)
{
@@ -70,7 +70,7 @@ RmgrCleanup(void)
continue;
if (RmgrTable[rmid].rm_cleanup != NULL)
- RmgrTable[rmid].rm_cleanup();
+ RmgrTable[rmid].rm_cleanup(in_redo);
}
}
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 39ef865ed9..008b439dd6 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1626,16 +1626,17 @@ PerformWalRecovery(void)
}
if (record != NULL)
+ InRedo = true;
+
+ RmgrStartup(InRedo);
+
+ if (InRedo)
{
TimestampTz xtime;
PGRUsage ru0;
pg_rusage_init(&ru0);
- InRedo = true;
-
- RmgrStartup();
-
ereport(LOG,
(errmsg("redo starts at %X/%X",
LSN_FORMAT_ARGS(xlogreader->ReadRecPtr))));
@@ -1771,8 +1772,6 @@ PerformWalRecovery(void)
}
}
- RmgrCleanup();
-
ereport(LOG,
(errmsg("redo done at %X/%X system usage: %s",
LSN_FORMAT_ARGS(xlogreader->ReadRecPtr),
@@ -1782,8 +1781,6 @@ PerformWalRecovery(void)
ereport(LOG,
(errmsg("last completed transaction was at log time %s",
timestamptz_to_str(xtime))));
-
- InRedo = false;
}
else
{
@@ -1792,6 +1789,9 @@ PerformWalRecovery(void)
(errmsg("redo is not required")));
}
+ RmgrCleanup(InRedo);
+ InRedo = false;
+
/*
* This check is intentionally after the above log messages that indicate
* how far recovery went.
diff --git a/src/include/access/ginxlog.h b/src/include/access/ginxlog.h
index 21de389d79..e4a91a8d6e 100644
--- a/src/include/access/ginxlog.h
+++ b/src/include/access/ginxlog.h
@@ -209,8 +209,8 @@ typedef struct ginxlogDeleteListPages
extern void gin_redo(XLogReaderState *record);
extern void gin_desc(StringInfo buf, XLogReaderState *record);
extern const char *gin_identify(uint8 info);
-extern void gin_xlog_startup(void);
-extern void gin_xlog_cleanup(void);
+extern void gin_xlog_startup(bool in_redo);
+extern void gin_xlog_cleanup(bool in_redo);
extern void gin_mask(char *pagedata, BlockNumber blkno);
#endif /* GINXLOG_H */
diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
index 4537e67eba..39f08a9611 100644
--- a/src/include/access/gistxlog.h
+++ b/src/include/access/gistxlog.h
@@ -107,8 +107,8 @@ typedef struct gistxlogPageReuse
extern void gist_redo(XLogReaderState *record);
extern void gist_desc(StringInfo buf, XLogReaderState *record);
extern const char *gist_identify(uint8 info);
-extern void gist_xlog_startup(void);
-extern void gist_xlog_cleanup(void);
+extern void gist_xlog_startup(bool in_redo);
+extern void gist_xlog_cleanup(bool in_redo);
extern void gist_mask(char *pagedata, BlockNumber blkno);
#endif
diff --git a/src/include/access/nbtxlog.h b/src/include/access/nbtxlog.h
index de362d3cb9..f6d05de06d 100644
--- a/src/include/access/nbtxlog.h
+++ b/src/include/access/nbtxlog.h
@@ -344,8 +344,8 @@ typedef struct xl_btree_newroot
extern void btree_redo(XLogReaderState *record);
extern void btree_desc(StringInfo buf, XLogReaderState *record);
extern const char *btree_identify(uint8 info);
-extern void btree_xlog_startup(void);
-extern void btree_xlog_cleanup(void);
+extern void btree_xlog_startup(bool in_redo);
+extern void btree_xlog_cleanup(bool in_redo);
extern void btree_mask(char *pagedata, BlockNumber blkno);
#endif /* NBTXLOG_H */
diff --git a/src/include/access/spgxlog.h b/src/include/access/spgxlog.h
index 930ffdd4f7..592ddb1d89 100644
--- a/src/include/access/spgxlog.h
+++ b/src/include/access/spgxlog.h
@@ -250,8 +250,8 @@ typedef struct spgxlogVacuumRedirect
extern void spg_redo(XLogReaderState *record);
extern void spg_desc(StringInfo buf, XLogReaderState *record);
extern const char *spg_identify(uint8 info);
-extern void spg_xlog_startup(void);
-extern void spg_xlog_cleanup(void);
+extern void spg_xlog_startup(bool in_redo);
+extern void spg_xlog_cleanup(bool in_redo);
extern void spg_mask(char *pagedata, BlockNumber blkno);
#endif /* SPGXLOG_H */
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index fae0bef8f5..058b2ef020 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -313,16 +313,16 @@ typedef struct RmgrData
void (*rm_redo) (XLogReaderState *record);
void (*rm_desc) (StringInfo buf, XLogReaderState *record);
const char *(*rm_identify) (uint8 info);
- void (*rm_startup) (void);
- void (*rm_cleanup) (void);
+ void (*rm_startup) (bool in_redo);
+ void (*rm_cleanup) (bool in_redo);
void (*rm_mask) (char *pagedata, BlockNumber blkno);
void (*rm_decode) (struct LogicalDecodingContext *ctx,
struct XLogRecordBuffer *buf);
} RmgrData;
extern PGDLLIMPORT RmgrData RmgrTable[];
-extern void RmgrStartup(void);
-extern void RmgrCleanup(void);
+extern void RmgrStartup(bool in_redo);
+extern void RmgrCleanup(bool in_redo);
extern void RmgrNotFound(RmgrId rmid);
extern void RegisterCustomRmgr(RmgrId rmid, RmgrData *rmgr);
view thread (13+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Comments on Custom RMGRs
In-Reply-To: <CANbhV-G7SfdRLR6r7sxY0uL=TRU0S=cxkWJGWyTdss3u8LFK=g@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox