rmgr hooks and contrib/rmgr_hook
As previously discussed on -hackers on Aug 19, "Proposed Resource
Manager Changes".
Enclosed are two closely related items:
1) A refactoring of calls to Rmgr code from xlog.c, and having isolated
the code for rmgrs then to allow rmgr plugins to modify and/or add rmgrs
to Postgres. Includes additional code to generate log messages so we can
see what is happening after plugin has executed.
Introduces a shared memory area for Rmgrs that allows each backend to
read which RmgrIds are valid for the currently running server, allowing
call to be made during XLogInsert() to validate rmgrid. (The validation
uses a fixed length BitMapSet, a minor new invention for this patch, but
that is begging to be refactored - I await advice and/or comments on the
fastest way to do this if that isn't it.)
(I'd like to rip out WAL_DEBUG completely in favour of this new
mechanism, but I haven't done that here).
2) contrib module that contains an example rmgr_hook - actually two
examples in one module
These have both been tested in normal mode, WAL_DEBUG mode and in warm
standby recovery, so not a WIP progress patch.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> wrote:
1) A refactoring of calls to Rmgr code from xlog.c, and having isolated
the code for rmgrs then to allow rmgr plugins to modify and/or add rmgrs
to Postgres. Includes additional code to generate log messages so we can
see what is happening after plugin has executed.
Why do we need to set rmgr_hook in _PG_init(), and add or mofify rmgrs
in our hook functions? I think it is possible to modify RmgrTable
directly in _PG_init() instead of to have rmgr_hook. If we can do so,
the patch would be more simple, no? Am I missing something?
Index: src/backend/access/transam/rmgr.c
===================================================================
--- src/backend/access/transam/rmgr.c (head)
+++ src/backend/access/transam/rmgr.c (new)
@@ -25,1 +25,1 @@
-const RmgrData RmgrTable[RM_MAX_ID + 1] = {
+RmgrData RmgrTable[MAX_NUM_RMGRS + 1] = {
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
On Mon, 2008-09-01 at 12:42 +0900, ITAGAKI Takahiro wrote:
Simon Riggs <simon@2ndQuadrant.com> wrote:
1) A refactoring of calls to Rmgr code from xlog.c, and having isolated
the code for rmgrs then to allow rmgr plugins to modify and/or add rmgrs
to Postgres. Includes additional code to generate log messages so we can
see what is happening after plugin has executed.Why do we need to set rmgr_hook in _PG_init(), and add or mofify rmgrs
in our hook functions? I think it is possible to modify RmgrTable
directly in _PG_init() instead of to have rmgr_hook. If we can do so,
the patch would be more simple, no? Am I missing something?
If we modify RmgrTable in _PG_init() then we would have to have that
structure available in all backends, which was a stated objective to
avoid. We would still need a fast access data structure for the
XLogInsert() check, so the RmgrTable would just be wasted space in all
normal backends. In the patch, plugin is only called when we call
RmgrInitialize(), so the memory is malloc'd only when required.
The other reason is that this way of doing things is common to most
other server hooks. It allows the loaded module to specify that there
will be a plugin, but for the server to determine when that is called.
Calling code at the time _PG_init() runs limits us to certain types of
activity.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> wrote:
Why do we need to set rmgr_hook in _PG_init(), and add or mofify rmgrs
in our hook functions?If we modify RmgrTable in _PG_init() then we would have to have that
structure available in all backends, which was a stated objective to
avoid. We would still need a fast access data structure for the
XLogInsert() check, so the RmgrTable would just be wasted space in all
normal backends. In the patch, plugin is only called when we call
RmgrInitialize(), so the memory is malloc'd only when required.
Um? AFAICS RmgrTable is not accessed in XLogInsert unless we use WAL_DEBUG.
I see that RmgrTable should be malloc'd when required,
but there is another issue; when to load rmgr libraries.
Rmgr objects are needed only in startup process during recovery.
If we want to reduce resource consumption by rmgrs, I think it is
better not to load rmgr libraries through shared_preload_libraries.
We don't have to load rmgr libs if recovery is not needed or after recovery.
How about adding a new variable "recovery_preload_libaries" like as
shared_preload_libraries? Rmgr libs in it are loaded only in startup
process and only if recovery is needed.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
On Tue, 2008-09-02 at 18:30 +0900, ITAGAKI Takahiro wrote:
Simon Riggs <simon@2ndQuadrant.com> wrote:
Why do we need to set rmgr_hook in _PG_init(), and add or mofify rmgrs
in our hook functions?If we modify RmgrTable in _PG_init() then we would have to have that
structure available in all backends, which was a stated objective to
avoid. We would still need a fast access data structure for the
XLogInsert() check, so the RmgrTable would just be wasted space in all
normal backends. In the patch, plugin is only called when we call
RmgrInitialize(), so the memory is malloc'd only when required.Um? AFAICS RmgrTable is not accessed in XLogInsert unless we use WAL_DEBUG.
Exactly why I want to malloc it.
I see that RmgrTable should be malloc'd when required,
but there is another issue; when to load rmgr libraries.
Rmgr objects are needed only in startup process during recovery.
If we want to reduce resource consumption by rmgrs, I think it is
better not to load rmgr libraries through shared_preload_libraries.
We don't have to load rmgr libs if recovery is not needed or after recovery.How about adding a new variable "recovery_preload_libaries" like as
shared_preload_libraries? Rmgr libs in it are loaded only in startup
process and only if recovery is needed.
Good point. If others agree, I will re-implement this way.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
ITAGAKI Takahiro wrote:
I see that RmgrTable should be malloc'd when required,
but there is another issue; when to load rmgr libraries.Rmgr objects are needed only in startup process during recovery.
If we want to reduce resource consumption by rmgrs, I think it is
better not to load rmgr libraries through shared_preload_libraries.
We don't have to load rmgr libs if recovery is not needed or after recovery.How about adding a new variable "recovery_preload_libaries" like as
shared_preload_libraries? Rmgr libs in it are loaded only in startup
process and only if recovery is needed.
It doesn't seem worth it to introduce a new GUC like that, just to
reduce the memory usage a tiny bit in the rare case that a rmgr plugin
is present. How much memory will loading an extra library consume
anyway? Depends on the library of course, but I believe we're talking
about something in the ballpark of a few hundred kb. Besides, a decent
OS should swap that to disk, if it's not used, and the system is tight
on memory.
Also, presumably the library containing the recovery functions, also
contains the functions that generate those WAL records. So, it will be
needed after startup anyway, if the plugin is used at all.
There's one more reason to use shared_preload_libraries. It provides a
sanity check that the library required for recovery is present and can
be loaded, even when no recovery is required. If you have misconfigured
your system so that it can't recover, you want to find out sooner rather
than later when recovery is needed.
So IMHO, just use shared_preload_libraries.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Tue, 2008-09-02 at 13:38 +0300, Heikki Linnakangas wrote:
There's one more reason to use shared_preload_libraries. It provides a
sanity check that the library required for recovery is present and
can
be loaded, even when no recovery is required. If you have
misconfigured
your system so that it can't recover, you want to find out sooner
rather
than later when recovery is needed.
Great reason.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
On Tue, 2008-09-02 at 18:30 +0900, ITAGAKI Takahiro wrote:
How about adding a new variable "recovery_preload_libaries" like as
shared_preload_libraries? Rmgr libs in it are loaded only in startup
process and only if recovery is needed.
Good point. If others agree, I will re-implement this way.
Aside from the objections raised by Heikki, I'm not seeing the use-case
for an rmgr that only executes during recovery; in fact I'm not entirely
sure that I see a use-case for this entire patch. Where are the WAL
records that the "loadable rmgr" processes going to come from?
regards, tom lane
On Tue, 2008-09-02 at 11:39 -0400, Tom Lane wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
On Tue, 2008-09-02 at 18:30 +0900, ITAGAKI Takahiro wrote:
How about adding a new variable "recovery_preload_libaries" like as
shared_preload_libraries? Rmgr libs in it are loaded only in startup
process and only if recovery is needed.Good point. If others agree, I will re-implement this way.
Aside from the objections raised by Heikki
Heikki hasn't raised any. He was objecting to an additional thought from
Itagaki. There haven't been any other objections to this concept.
, I'm not seeing the use-case
for an rmgr that only executes during recovery; in fact I'm not entirely
sure that I see a use-case for this entire patch. Where are the WAL
records that the "loadable rmgr" processes going to come from?
There is ample reason to do this. I covered this in my first post,
please re-read up thread. You have commented on this post already, so
I'm suprised by your comments.
Rmgr functions only execute during recovery, that is their role in life.
Except when we have WAL_DEBUG enabled they are never called elsewhere.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Simon Riggs wrote:
On Tue, 2008-09-02 at 11:39 -0400, Tom Lane wrote:
, I'm not seeing the use-case
for an rmgr that only executes during recovery; in fact I'm not entirely
sure that I see a use-case for this entire patch. Where are the WAL
records that the "loadable rmgr" processes going to come from?There is ample reason to do this. I covered this in my first post,
please re-read up thread. You have commented on this post already, so
I'm suprised by your comments.
I think there's two different use cases here:
1. Filter WAL that's already generated, or is being generated by an
unmodified PostgreSQL instance.
2. Allow external modules to define new resource managers.
The examples you posted with the patch were of type 1. That's a very
valid use case, the example of only restoring a single database seems
like a useful tool. Another tool like that is pglesslog, although that
one couldn't actually be implemented with these hooks. I'm sure there's
more tricks like that people would find useful, if the tools existed.
The importance of the WAL will only increase as more people start to use
it for PITR, replication etc.
The 2nd use case, however, I find pretty unconvincing. I can't think of
a good example of that. Anything that needs to define its own resource
manager is very low-level stuff, and probably needs to go into the core
anyway.
So, let's focus on the 1st use case. I think a better approach for that
is to implement the filters as external programs, like pglesslog. It
allows more flexibility, although it also means that you can't rely on
existing backend functions to manipulate the WAL. I'd love to see a "WAL
toolkit" on pgfoundry, with tools like the filter to only restore a
single database, pglesslog, a WAL record viewer etc. A while ago, you
also talked about replacing the Slony triggers in the master with a tool
that reads the WAL; another good example of an external tool that needs
to read WAL. The toolkit could provide some sort of a framework and
common user interface to read and write WAL files for all those tools.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Sat, 2008-09-13 at 10:59 +0300, Heikki Linnakangas wrote:
The importance of the WAL will only increase as more people start to
use it for PITR, replication etc.
Agreed.
The 2nd use case, however, I find pretty unconvincing. I can't think of
a good example of that. Anything that needs to define its own resource
manager is very low-level stuff, and probably needs to go into the core
anyway.
New indexes are a big one, but I listed others also.
Indexes have always been able to be added dynamically. Now they can be
recovered correctly as well.
Other data structures can be maintained by trigger code that writes new
types of WAL. That was always possible before, now they can be
recoverable too.
If we have extensible functions, triggers, indexes, why not WAL? What is
the problem with making WAL extensible? It carries no penalty at all for
standard WAL records, since the internal design for WAL already caters
for exactly this.
So, let's focus on the 1st use case.
No, lets look at both...you can't just wave away half the use cases. If
you look at all of the use cases the argument for doing it externally
quickly falls apart since it severely limits what can be achieved.
I think a better approach for that
is to implement the filters as external programs, like pglesslog. It
allows more flexibility, although it also means that you can't rely on
existing backend functions to manipulate the WAL. I'd love to see a "WAL
toolkit" on pgfoundry, with tools like the filter to only restore a
single database, pglesslog, a WAL record viewer etc. A while ago, you
also talked about replacing the Slony triggers in the master with a tool
that reads the WAL; another good example of an external tool that needs
to read WAL. The toolkit could provide some sort of a framework and
common user interface to read and write WAL files for all those tools.
This patch provides exactly the toolkit you describe, just internally.
As you point out, doing it other ways means you can't access internal
functions easily and can't maintain internal data structures correctly
either. So doing it externally is *not* a substitute and this is not a
simple discussion of include/exclude from core.
I'm lost as to why suggesting we limit the functionality is going to be
a good thing? If external tools really are so good, then we can do that
*as well*.
But this is only a plugin API, so the tools will be developed externally
anyway.
A while ago, you
also talked about replacing the Slony triggers in the master with a tool
that reads the WAL
Writes the WAL you mean? Slony triggers could write data to WAL rather
than log tables and the slon daemon can be implemented as an rmgr
plugin. Or many other options.
Another tool like that is pglesslog, although that
one couldn't actually be implemented with these hooks.
Sounds like we'll want to integrate that into synch replication some
how, so suggestions as to how to do that welcome - if you're not already
doing it via some other plugin in synch rep code?
I'm sure there's
more tricks like that people would find useful, if the tools existed.
Agreed. So lets make them exist.
If there's an argument against doing this, I've not heard it made
clearly by anybody. When we discussed it first on hackers there was no
objection, so I wrote the patch. If people want to see this blocked now,
we need some good reasons.
I've got nothing riding on the acceptance of this patch, I just think
its a good thing. That's why I deprioritised it. If there's some hidden
threat to national security or whatever, tell me off list.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
On Sat, 2008-09-13 at 10:59 +0300, Heikki Linnakangas wrote:
The 2nd use case, however, I find pretty unconvincing. I can't think of
a good example of that. Anything that needs to define its own resource
manager is very low-level stuff, and probably needs to go into the core
anyway.New indexes are a big one, but I listed others also.
Indexes have always been able to be added dynamically. Now they can be
recovered correctly as well.
Hm, so currently if you want to add a new indexam you can't just insert into
pg_am and make them recoverable. You basically have to build in your new index
access method into Postgres with the new rmgr. That is annoying and a problem
worth tackling.
But I'm a bit worried about having this be an external plugin. There's no way
to looking at a WAL file to know whether it will be recoverable with the
plugins available. Worse, there's a risk you could have a plugin but not the
*right* plugin. Perhaps this could be tackled simply by having startup insert
a record listing all the rmgr's in use with identifying information and their
version numbers.
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's PostGIS support!
On Mon, 2008-09-15 at 10:47 +0100, Gregory Stark wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
On Sat, 2008-09-13 at 10:59 +0300, Heikki Linnakangas wrote:
The 2nd use case, however, I find pretty unconvincing. I can't think of
a good example of that. Anything that needs to define its own resource
manager is very low-level stuff, and probably needs to go into the core
anyway.New indexes are a big one, but I listed others also.
Indexes have always been able to be added dynamically. Now they can be
recovered correctly as well.Hm, so currently if you want to add a new indexam you can't just insert into
pg_am and make them recoverable. You basically have to build in your new index
access method into Postgres with the new rmgr. That is annoying and a problem
worth tackling.
Agreed.
But I'm a bit worried about having this be an external plugin. There's no way
to looking at a WAL file to know whether it will be recoverable with the
plugins available. Worse, there's a risk you could have a plugin but not the
*right* plugin.
That risk was discussed and is handled in the plugin. You are limited to
only insert data into WAL that has a current plugin that says it will
handle redo for that type.
Perhaps this could be tackled simply by having startup insert
a record listing all the rmgr's in use with identifying information and their
version numbers.
Non-standard plugins in use are listed when in use, so we can all see
what's going on. Plugins can issue their own startup messages if they
choose, with version numbers and other details.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Gregory Stark <stark@enterprisedb.com> writes:
Simon Riggs <simon@2ndQuadrant.com> writes:
Indexes have always been able to be added dynamically. Now they can be
recovered correctly as well.
Hm, so currently if you want to add a new indexam you can't just insert into
pg_am and make them recoverable. You basically have to build in your new index
access method into Postgres with the new rmgr. That is annoying and a problem
worth tackling.
I concur with Heikki that that's not exactly a compelling use-case.
I've never heard of anyone building a non-core index AM at all; much
less trying to use it in a production context. Given the obvious
potential for version-mismatch-type problems, it's hard to believe
that anyone ever would try.
regards, tom lane
On Mon, 2008-09-15 at 08:52 -0400, Tom Lane wrote:
Gregory Stark <stark@enterprisedb.com> writes:
Simon Riggs <simon@2ndQuadrant.com> writes:
Indexes have always been able to be added dynamically. Now they can be
recovered correctly as well.Hm, so currently if you want to add a new indexam you can't just insert into
pg_am and make them recoverable. You basically have to build in your new index
access method into Postgres with the new rmgr. That is annoying and a problem
worth tackling.I concur with Heikki that that's not exactly a compelling use-case.
I've never heard of anyone building a non-core index AM at all; much
less trying to use it in a production context. Given the obvious
potential for version-mismatch-type problems, it's hard to believe
that anyone ever would try.
The lack of a chicken is not an argument against the use case for an
egg.
But in any case, Bizgres was exactly this case, so they already did. We
just forced the authors to produce a code fork to do it, confusing
people rather than attracting people to Postgres.
We have plugin APIs with possible version mismatches in other contexts,
but I don't see us doing anything about that. In the context of WAL, the
basic WAL format has not changed in about 6 releases, so its been one of
the most stable file formats. Certain message types have changed, but
messages are all independent across rmgrs, so insulated from change.
The version mismatch idea presumes that a code author would structure
their code in two pieces: one to generate the WAL and one to read it.
Seems much more likely to me that authors would have one module
containing both as a way of avoiding the problem altogether. So I'm not
sure what to check, and against what?
When people do write useful plugins in the future they will be
potentially usable with any server at 8.4 or above. If we had had this
feature a few releases ago, we could have made GIN available to earlier
releases, for example.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Tom Lane <tgl@sss.pgh.pa.us> writes:
Gregory Stark <stark@enterprisedb.com> writes:
Simon Riggs <simon@2ndQuadrant.com> writes:
Indexes have always been able to be added dynamically. Now they can be
recovered correctly as well.Hm, so currently if you want to add a new indexam you can't just insert into
pg_am and make them recoverable. You basically have to build in your new index
access method into Postgres with the new rmgr. That is annoying and a problem
worth tackling.I concur with Heikki that that's not exactly a compelling use-case.
I've never heard of anyone building a non-core index AM at all; much
less trying to use it in a production context. Given the obvious
potential for version-mismatch-type problems, it's hard to believe
that anyone ever would try.
Well wasn't GIST such an instance until we put it in core? IIRC it lived in
contrib for a long time. It happens that the route they took was to implement
it without recoverability until it was in core then add logging. I suspect we
would lean on any new method to have logging before it was merged in though.
I think the version-mismatch problems are fairly important though which is why
I was suggesting providing checks for that in postgres. Simon's right though
that the plugin could check for it itself.
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!
Simon Riggs <simon@2ndQuadrant.com> writes:
The version mismatch idea presumes that a code author would structure
their code in two pieces: one to generate the WAL and one to read it.
Seems much more likely to me that authors would have one module
containing both as a way of avoiding the problem altogether. So I'm not
sure what to check, and against what?
No, the danger is that someone generates a backup with one version of the
plugin and then restores with a different version of the plugin.
That would be frightfully easy to do when doing a minor upgrade, for example.
Or on a standby database if you've installed a new version of the plugin since
the standby was built.
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's PostGIS support!
Simon Riggs wrote:
On Mon, 2008-09-15 at 08:52 -0400, Tom Lane wrote:
I've never heard of anyone building a non-core index AM at all; much
less trying to use it in a production context. Given the obvious
potential for version-mismatch-type problems, it's hard to believe
that anyone ever would try.The lack of a chicken is not an argument against the use case for an
egg.But in any case, Bizgres was exactly this case, so they already did. We
just forced the authors to produce a code fork to do it, confusing
people rather than attracting people to Postgres.
Are you referring to the bitmap index patch? IIRC, there was some
non-trivial changes to indexam API in there, as well as issues with
VACUUM. If anything, that patch supports the assumption that anything
that needs WAL-logging is working at such a low-level that it needs to
be in core anyway.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Simon Riggs <simon@2ndQuadrant.com> writes:
We have plugin APIs with possible version mismatches in other contexts,
but I don't see us doing anything about that. In the context of WAL, the
basic WAL format has not changed in about 6 releases, so its been one of
the most stable file formats.
Er, that's simply false. Read the revision history for xlog_internal.h.
The version mismatch idea presumes that a code author would structure
their code in two pieces: one to generate the WAL and one to read it.
No, the version mismatch problem is that you might try to read the WAL
with a different version of the plugin than you wrote it with. Or maybe
with a completely unrelated plugin that was unfortunate enough to choose
the same rmgr ID. We can't afford to insert complete versioning
information into each WAL record, so it's going to be pretty difficult
to avoid this risk.
When people do write useful plugins in the future they will be
potentially usable with any server at 8.4 or above. If we had had this
feature a few releases ago, we could have made GIN available to earlier
releases, for example.
Well, the initial commit for GIN looked like this:
2006-05-02 07:28 teodor
* contrib/tsearch2/Makefile, contrib/tsearch2/ginidx.c,
contrib/tsearch2/tsearch.sql.in,
contrib/tsearch2/expected/tsearch2.out,
contrib/tsearch2/sql/tsearch2.sql, src/backend/access/Makefile,
src/backend/access/gin/Makefile, src/backend/access/gin/README,
src/backend/access/gin/ginarrayproc.c,
src/backend/access/gin/ginbtree.c,
src/backend/access/gin/ginbulk.c,
src/backend/access/gin/gindatapage.c,
src/backend/access/gin/ginentrypage.c,
src/backend/access/gin/ginget.c,
src/backend/access/gin/gininsert.c,
src/backend/access/gin/ginscan.c, src/backend/access/gin/ginutil.c,
src/backend/access/gin/ginvacuum.c,
src/backend/access/gin/ginxlog.c,
src/backend/access/transam/rmgr.c, src/backend/commands/cluster.c,
src/backend/commands/opclasscmds.c, src/backend/commands/vacuum.c,
src/backend/utils/adt/selfuncs.c, src/backend/utils/init/globals.c,
src/backend/utils/misc/guc.c, src/include/access/gin.h,
src/include/access/rmgr.h, src/include/catalog/catversion.h,
src/include/catalog/pg_am.h, src/include/catalog/pg_amop.h,
src/include/catalog/pg_amproc.h, src/include/catalog/pg_opclass.h,
src/include/catalog/pg_operator.h, src/include/catalog/pg_proc.h,
src/include/utils/selfuncs.h, src/test/regress/data/array.data,
src/test/regress/expected/arrays.out,
src/test/regress/expected/create_index.out,
src/test/regress/expected/create_table.out,
src/test/regress/expected/opr_sanity.out,
src/test/regress/expected/sanity_check.out,
src/test/regress/input/copy.source,
src/test/regress/output/copy.source,
src/test/regress/output/misc.source,
src/test/regress/sql/arrays.sql,
src/test/regress/sql/create_index.sql,
src/test/regress/sql/create_table.sql,
src/test/regress/sql/opr_sanity.sql: GIN: Generalized Inverted
iNdex. text[], int4[], Tsearch2 support for GIN.
Had the only core source file touched been rmgr.c, then maybe this
argument would be valid ...
regards, tom lane
On Mon, 2008-09-15 at 10:04 -0400, Tom Lane wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
The version mismatch idea presumes that a code author would structure
their code in two pieces: one to generate the WAL and one to read it.No, the version mismatch problem is that you might try to read the WAL
with a different version of the plugin than you wrote it with. Or maybe
with a completely unrelated plugin that was unfortunate enough to choose
the same rmgr ID. We can't afford to insert complete versioning
information into each WAL record, so it's going to be pretty difficult
to avoid this risk.
I'm happy to include additional things into the patch, but I don't see
anything to force rejection of the patch entirely, from what has been
said.
Bottom line is that any backup of Postgres needs to include plugin
directories, otherwise parts of the application could stop working
following restore. This patch doesn't change that.
I proposed a registration scheme to avoid one of those problems.
If a plugin changed its file format, it would clearly need to include a
version test within it. It wouldn't be the fault of the plugin API if
the plugin author didn't handle that. But if they can work out how to
build an index AM and write WAL, I'm sure they can handle version
management between software components and informative error messages if
problems occur. And if they can't, they'll get a bad rep and nobody will
use the plugin.
Few ideas:
* add the rmgr bms to the long header of each WAL file
* change !RmgrIdIsValid() so that it causes FATAL by default. This then
allows people to correct a mistake and retry. We provide an option to
treat such errors as corrupt data and assume we have reached the end of
WAL.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support