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

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

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

>>> 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
f5e524d92be609c709825be8995bf77f10880c3b
47082fa875179ae629edb26807ab3f38a775280b

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL