Re: adding a new column in IDENTIFY_SYSTEM

On Mon, May 16, 2011 at 01:03, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Thu, May 5, 2011 at 10:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>> So even if people don't believe in the rationale behind the patch,
>>>> would allowing it harm anything at this point?
>>
>>> Adding it for the sake of upgrades seems very far fetched.
>>
>>> Adding it for the sake of giving a better error message seems like a
>>> very good idea. But in that case, the client side code to actually
>>> give a better error message should be included from the start, IMHO.
>>
>> What's not apparent to me is how we'll even get to this check; if
>> there's a mismatch, won't the database system identifier comparison
>> fail first in most scenarios?
>>
>
> that's why i didn't propose that to begin with... but thinking on
> that, we can use it to add a message in pg_basebackup, maybe just a
> warning if we are taking a basebackup from an incompatible system...
>
> but for that i will need to add xlog_internal.h and postgres.h to
> pg_basebackup and use the "#define FRONTEND 1" hack we have in
> pg_resetxlog

Well, pg_basebackup doesn't need it critically, since it never looks
at the contents fo the files anyway. You could use a pg_basebackup for
9.1 to backup a 9.2 database - at least in theory.

Granted, it wouldn't hurt to get the message from pg_basebackup
*before* you took the backup, which your patch (from the other email)
does. But I'm not entirely sure I like that kludge... I think it'd be
less of a kludge to move the definition of XLOG_PAGE_MAGIC somewhere
that's visible already.

Also, this error message:
+ fprintf(stderr, _("%s: could not identify system: XLOG
pages are incompatible.\n"),

is clearly wrong - it *could* identify the system, it just didn't like
what it saw...

Anyway, the more useful point would be to have it in walreceiver, I believe.

>> I'm also wondering why send WAL version number and not, say, catalog
>> version number, if there's some idea that we need more tests than the
>> system identifier comparison.
>>
>
> well... catversion is not that informative, we change it for a lot of
> reasons, not only catalog estructure changes... so we can't swear that
> xlog records will be incompatible just because catversion changes...

From the *replication* perspective we can be pretty certain it breaks.
From the base backup perspective, it might well keep on working, since
you get the new version of both the base backup and the logs.

And what other reasons than catalog structure changes do we actually
change catversion?

> but yes, we need to know if catalog estructure has changed, maybe we
> can change XLOG_PAGE_MAGIC when that happens?

Uh, that seems like a seriously bad idea.

>> Given reasonable answers to these questions, I'd not object to putting
>> in additional error testing.  I concur with Magnus that the patch should
>> actually provide those tests, and not just put in an unused field.
>>
>
> actually, now is when we can play with that API at will when/if we can
> make online upgrades work then we will be stuck with whatever we have
> made. before that we know it won't affect anybody

True - but there should be at least a POC implenentation of something
*using* the API, or we don't know if it's useful. As stated earlier,
I'd prefer that POC API to be the walreceiver.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/