Обсуждение: Re: rmgr hooks and contrib/rmgr_hook

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.comPostgreSQL 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.comPostgreSQL 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.comPostgreSQL 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.comPostgreSQL 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.comPostgreSQL 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

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!

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.comPostgreSQL Training, Services and 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 InvertediNdex.    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.comPostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> writes:

> 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.

No, backups of executables are normally not the same backups as the data and
in many cases -- minor upgrades for example -- cannot be.

> * 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.

I'm not sure but I think this just begs the question. The problem is to ensure
that the rmgrid means the same thing on the restoring database as it does on
the original database, or at least a compatible version. I think this would
mean having a long text description and version number to compare.

And as Tom points out startup isn't often enough. Would WAL headers even be
often enough? We would have to ensure there was never two versions of the
plugin in the same WAL file.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production
Tuning

On Mon, 2008-09-15 at 16:43 +0100, Gregory Stark wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> 
> > 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.
> 
> No, backups of executables are normally not the same backups as the data and
> in many cases -- minor upgrades for example -- cannot be.

So you advise your clients to do backup in two halves. Then complain
that this is a bad way to do a backup because it may cause
insurmountable problems on restore. And then seek to reject a patch
because of that, even though similar problems already exist in other
parts of the system. I'm sorry, but that is circular, then faulted
logic.

If you do a minor upgrade that changes the on-disk format of *any* part
of the system then you have just introduced a limitation into what
software can be used for restore. That could be a new version of a
custom datatype just as easily as it could be an rmgr plugin. Shall we
ban custom datatypes? Should we add a version number into every varlen
header just in case somebody switched release levels, then forgot?

> > * 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.
> 
> I'm not sure but I think this just begs the question. The problem is to ensure
> that the rmgrid means the same thing on the restoring database as it does on
> the original database, or at least a compatible version. I think this would
> mean having a long text description and version number to compare.

Why is that any different to using functions or other plugins? If you
restore data into a database where the functions have the same names,
yet do different things then you are in trouble. Period.

If you don't use an rmgr plugin at all then you have nothing different
to do, nor will you see any different messages. If you use *any*
external server software, expect to read the instructions or have
problems.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

On Mon, 2008-09-15 at 16:00 +0100, Simon Riggs wrote:
> 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.

There are a few cases we might be concerned about:

* rmgr plugin using duplicate id
* rmgr plugin using wrong id
* missing rmgr plugin
* version mismatch on WAL format of Rmgr record
* rmgr plugin wrong version to server version

We can handle those issues like this

* rmgr plugin using duplicate id

Registration scheme avoids this somewhat. Plugin setup disallows
possibility of multiple plugins with same id. Ranges of values have been
assigned so we advise people which values to use. This is same issue as
IANA for well known port numbers.

* rmgr plugin using wrong id

We can't actually force code to make calls using the right RmgrId. So
the best we can do is tell the plugin what number it has been assigned
to and have it throw an error if it doesn't expect that number and can't
dynamically change its RmgrId. The latter is just too complex too expect
people to do, so hardcoding the RmgrId is the expected way. That,
combined with the registration scheme should be sufficient. We aren't
expecting more than a 10-20 of these anyway. Again, similar to TCP/IP -
not all software knows how to operate on different port numbers.

*  missing rmgr plugin

It seems sufficient to make the test for !RmgrIdIsValid return FATAL.
This then allows people to correct mistakes and retry. By the time this
test happens the WAL record has been CRC checked and has a valid header
in all other ways. We can document how to put in a dummy handler if the
rmgr plugin has bugs that need to be worked around to continue recovery.

* version mismatch on WAL format of Rmgr record

Immediately following the "shutdown" checkpoint at startup we should
write out a version string for all rmgr plugins as a WAL record. On
replay xlog_redo() will read the WAL record and test it against the
version information presented by the current plugin. If there is a
mismatch, we throw a FATAL error. This stops replay exactly on a
checkpoint record, so if you change rmgr to next version and restart
then no problems ensue. We allow new rmgr plugins that were not there
previously. These changes allow you to rollforward past a change in rmgr
WAL format. This then allows a standby server to be upgraded immediately
following a master server when using log shipping replication even when
the rmgr plugin changes its WAL format.

We don't go to that trouble for datatype versions, perhaps we should.
Overall, version management of rmgr plugins is same difficulty and can
result in errors of same severity as existing PostgresSQL extensions. So
there seems to be no additional risk from allowing them, in general.

We never expect to handle WAL changes across major versions and we will
increase the version numbers of all standard rmgrs at major releases, by
keying them to version numbers/strings already in use. WAL format
changes will be strongly discouraged for any single release.

* rmgr plugin wrong version to server version

Plugins are expected to test whether they are operating at the correct
version for the server. This information is already available if the
plugin wishes to act correctly.

So to do all of the above we need 3 changes:
* !RmgrIdIsValid throws FATAL not ERROR in ReadRecord()
* Alter rmgr API call to send RmgrId to plugin so it can throw error or
handle it somehow.In most cases this will lead to an plugin error "must
be defined on RmgrId 125" or similar.
* New rmgr API call to getRmgrVersion() so we can test it at startup
shutdown checkpoints.

It will take some time to make and test these changes, so this patch
should be marked returned with feedback from this commitfest.

If there are some more error modes, please say. I can't see any others.

This patch is about flexibility for the future. I have no pressing need
for this to go into 8.4, I just think its a good thing if it does. If
people think it poses a genuine risk or that there is insufficient time
to properly assess risks, then we can withdraw it.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support