Re: 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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)enterprisedb(dot)com> writes:
> Simon Riggs <simon(at)2ndQuadrant(dot)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(at)enterprisedb(dot)com> writes:
> > Simon Riggs <simon(at)2ndQuadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Gregory Stark <stark(at)enterprisedb(dot)com> writes:
>> Simon Riggs <simon(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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

Simon Riggs <simon(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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.com
PostgreSQL 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(at)2ndQuadrant(dot)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.com
PostgreSQL Training, Services and Support