Re: adding a new column in IDENTIFY_SYSTEM - Mailing list pgsql-hackers

On Tue, May 17, 2011 at 16:38, Jaime Casanova <jaime@2ndquadrant.com> wrote:
> On Mon, May 16, 2011 at 2:35 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Mon, May 16, 2011 at 01:03, Jaime Casanova <jaime@2ndquadrant.com> wrote:
>>> On Thu, May 5, 2011 at 10:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Magnus Hagander <magnus@hagander.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,
>
> while you could, is also possible that you really think is the right
> version and that you will waste time until you found out you have the
> wrong version installed and that your backup won't work

Yes. It might be useful to note it, and then ust make an override
flag. My pointm, though, was that doing it for walreceiver is more
important and a more logical first step.


>> I think it'd be
>> less of a kludge to move the definition of XLOG_PAGE_MAGIC somewhere
>> that's visible already.
>>
>
> agree, that also will allow us to avoid have that kludge in pg_resetxlog...
>
>> 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...
>>
>
> ah! yeah! we can, of course, put better messages!
>
>>
>> Anyway, the more useful point would be to have it in walreceiver, I believe.
>>
>
> you mean a message like this in walreceiver? we can put it but
> probably it will never get to that...

Yes, I think that's the more important one. And that without that, I
don't really see why we should do this for 9.1.


>>>> 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?
>>
>
> see these commits:
> 76dd09bbec893c02376e3440a6a86a3b994d804c

That's bruce... :-)

> f5e524d92be609c709825be8995bf77f10880c3b

That does have catalog changes. Not structure, but contents, which
would cause similar problems, no?

> 47082fa875179ae629edb26807ab3f38a775280b

That's a catversion bump because it was forgotten in a *previous*
patch that required it.

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