Обсуждение: pgsql-server/src/bin/pg_dump pg_backup_archiver.c
CVSROOT: /cvsroot
Module name: pgsql-server
Changes by: inoue@postgresql.org 03/01/03 13:05:02
Modified files:
src/bin/pg_dump: pg_backup_archiver.c
Log message:
Adjust lo type in contrib during pg_restore so that pg_restore could
reload the type.
inoue@postgresql.org (Hiroshi Inoue) writes:
> src/bin/pg_dump: pg_backup_archiver.c
> Log message:
> Adjust lo type in contrib during pg_restore so that pg_restore could
> reload the type.
I find this really ugly, and I do not believe this code belongs in
pg_dump (nor pg_restore). We are not in the habit of putting kluges
into pg_dump to adjust system catalog entries for version-to-version
changes, and I certainly don't think that we should put in such a kluge
for a type that's only contrib.
The correct way to update contrib/lo for 7.3 is to load the 7.3
definition into a database before restoring the old definition.
If someone fails to do that, it'd be okay to supply this fix as a script
in contrib/lo to run against the database after-the-fact. But I object
to putting it into pg_restore.
regards, tom lane
Tom Lane writes: > The correct way to update contrib/lo for 7.3 is to load the 7.3 > definition into a database before restoring the old definition. I'm not completely sure what the "lo" type provides, but if there was a cast between oid and lo in 7.2 (by having an appropriately named function), then pg_dump 7.3 should create the appropriate CREATE CAST statements for restore into a 7.3 database. Maybe pg_dump is missing this for some reason? -- Peter Eisentraut peter_e@gmx.net
Peter Eisentraut <peter_e@gmx.net> writes:
> I'm not completely sure what the "lo" type provides, but if there was a
> cast between oid and lo in 7.2 (by having an appropriately named
> function), then pg_dump 7.3 should create the appropriate CREATE CAST
> statements for restore into a 7.3 database. Maybe pg_dump is missing this
> for some reason?
I tried installing 7.2's contrib/lo and then running current pg_dump.
I see:
CREATE FUNCTION oid (lo) RETURNS oid
AS '$libdir/lo', 'lo_oid'
LANGUAGE "C";
CREATE CAST (lo AS oid) WITH FUNCTION oid (lo);
CREATE FUNCTION lo (oid) RETURNS lo
AS '$libdir/lo', 'lo'
LANGUAGE "C";
CREATE CAST (oid AS lo) WITH FUNCTION lo (oid);
The trouble with this is that the CREATE CASTs will fail because of the
prohibition against volatile cast functions.
I'm still of the opinion that that prohibition is inappropriate and
useless. I'd vote for removing it.
regards, tom lane
Tom Lane wrote:
>
> inoue@postgresql.org (Hiroshi Inoue) writes:
> > src/bin/pg_dump: pg_backup_archiver.c
>
> > Log message:
> > Adjust lo type in contrib during pg_restore so that pg_restore could
> > reload the type.
>
> I find this really ugly, and I do not believe this code belongs in
> pg_dump (nor pg_restore). We are not in the habit of putting kluges
> into pg_dump to adjust system catalog entries for version-to-version
> changes, and I certainly don't think that we should put in such a kluge
> for a type that's only contrib.
>
> The correct way to update contrib/lo for 7.3 is to load the 7.3
> definition into a database before restoring the old definition.
>
> If someone fails to do that, it'd be okay to supply this fix as a script
> in contrib/lo to run against the database after-the-fact. But I object
> to putting it into pg_restore.
Please tell me how we avoid the failure
ERROR: Unable to identify an operator '=' for types 'oid' and 'lo'
You will have to retype this query using an explicit cast
in pg_restore.
regards,
Hiroshi Inoue
http://w2422.nsk.ne.jp/~inoue/
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Please tell me how we avoid the failure
> ERROR: Unable to identify an operator '=' for types 'oid' and 'lo'
> You will have to retype this query using an explicit cast
> in pg_restore.
What query triggers that, exactly?
regards, tom lane
Tom Lane wrote:
>
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Please tell me how we avoid the failure
> > ERROR: Unable to identify an operator '=' for types 'oid' and 'lo'
> > You will have to retype this query using an explicit cast
> > in pg_restore.
>
> What query triggers that, exactly?
The query made by the following code in FixupBlobRefs in pg_backup_db.c.
/* Can't use fmtId twice in one call... */
appendPQExpBuffer(tblQry,
"UPDATE %s SET %s =
%s.newOid"
,
tblName->data,
fmtId(attr),
BLOB_XREF_TABLE);
appendPQExpBuffer(tblQry,
" FROM %s WHERE
%s.oldOid = %s.%s",
BLOB_XREF_TABLE,
BLOB_XREF_TABLE,
tblName->data,
fmtId(attr));
regards,
Hiroshi Inoue
http://w2422.nsk.ne.jp/~inoue/
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Tom Lane wrote:
>>
>> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Please tell me how we avoid the failure
> ERROR: Unable to identify an operator '=' for types 'oid' and 'lo'
> You will have to retype this query using an explicit cast
> in pg_restore.
>>
>> What query triggers that, exactly?
> The query made by the following code in FixupBlobRefs in pg_backup_db.c.
Hmmm ... does it help if we change the query to
" FROM %s WHERE
%s.oldOid = %s.%s::pg_catalog.oid",
I suspect though that the real issue is the CREATE CAST is failing;
if so, this won't help. See my comment to Peter earlier today.
regards, tom lane
Tom Lane wrote:
>
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Tom Lane wrote:
> >>
> >> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Please tell me how we avoid the failure
> > ERROR: Unable to identify an operator '=' for types 'oid' and 'lo'
> > You will have to retype this query using an explicit cast
> > in pg_restore.
> >>
> >> What query triggers that, exactly?
>
> > The query made by the following code in FixupBlobRefs in pg_backup_db.c.
>
> Hmmm ... does it help if we change the query to
>
> " FROM %s WHERE
> %s.oldOid = %s.%s::pg_catalog.oid",
>
> I suspect though that the real issue is the CREATE CAST is failing;
> if so, this won't help. See my comment to Peter earlier today.
I know it from the first and the consideration is useless
unless we use 7.3 pg_dump.
regards,
Hiroshi Inoue
http://w2422.nsk.ne.jp/~inoue/
> -----Original Message----- > From: Hiroshi Inoue > > Tom Lane wrote: > > > > Hiroshi Inoue <Inoue@tpf.co.jp> writes: > > > Tom Lane wrote: > > >> > > >> Hiroshi Inoue <Inoue@tpf.co.jp> writes: > > > Please tell me how we avoid the failure > > > ERROR: Unable to identify an operator '=' for types 'oid' and 'lo' > > > You will have to retype this query using an explicit cast > > > in pg_restore. > > >> > > >> What query triggers that, exactly? > > > > > The query made by the following code in FixupBlobRefs in > pg_backup_db.c. > > > > Hmmm ... does it help if we change the query to > > > > " FROM %s WHERE > > %s.oldOid = %s.%s::pg_catalog.oid", > > > > I suspect though that the real issue is the CREATE CAST is failing; > > if so, this won't help. See my comment to Peter earlier today. > > I know it from the first and the consideration is useless > unless we use 7.3 pg_dump. If there's no objection, I would apply the fix to 7.3-STABLE tree. regards, Hiroshi Inoue
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> If there's no objection, I would apply the fix to 7.3-STABLE tree.
I didn't like the pg_dump hack, and would rather we removed the prohibition
against creating casts using volatile functions.
regards, tom lane
> -----Original Message----- > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > > "Hiroshi Inoue" <Inoue@tpf.co.jp> writes: > > If there's no objection, I would apply the fix to 7.3-STABLE tree. > > I didn't like the pg_dump hack, and would rather we removed the > prohibition > against creating casts using volatile functions. I don't object to it but why should we use 7.3 pg_dump to dump the 7.2 or earlier database ? regards, Hiroshi Inoue
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>> I didn't like the pg_dump hack, and would rather we removed the
>> prohibition
>> against creating casts using volatile functions.
> I don't object to it but why should we use 7.3 pg_dump to dump
> the 7.2 or earlier database ?
That's one reason that I didn't like solving it by hacking pg_dump.
regards, tom lane
> -----Original Message----- > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > > "Hiroshi Inoue" <Inoue@tpf.co.jp> writes: > >> I didn't like the pg_dump hack, and would rather we removed the > >> prohibition > >> against creating casts using volatile functions. > > > I don't object to it but why should we use 7.3 pg_dump to dump > > the 7.2 or earlier database ? > > That's one reason that I didn't like solving it by hacking pg_dump. ???? My fix works well with the scenario 7.2 pg_restore -> 7.3 pg_restore. It's another problem that 7.3 pg_dump -> 7.3 pg_restore fails. regards, Hiroshi Inoue
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>> That's one reason that I didn't like solving it by hacking pg_dump.
> My fix works well with the scenario 7.2 pg_restore -> 7.3 pg_restore.
> It's another problem that 7.3 pg_dump -> 7.3 pg_restore fails.
Perhaps we're talking at cross-purposes. Exactly what was the failure
that your fix was intended to prevent? I thought the problem really
came down to the fact that reloading 7.2 "lo" type definitions into 7.3
would fail.
regards, tom lane
> -----Original Message----- > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > > "Hiroshi Inoue" <Inoue@tpf.co.jp> writes: > >> That's one reason that I didn't like solving it by hacking pg_dump. > > > My fix works well with the scenario 7.2 pg_restore -> 7.3 pg_restore. > > It's another problem that 7.3 pg_dump -> 7.3 pg_restore fails. > > Perhaps we're talking at cross-purposes. Exactly what was the failure > that your fix was intended to prevent? I thought the problem really > came down to the fact that reloading 7.2 "lo" type definitions into 7.3 > would fail. Sorry the first scenario is 7.2 pg_dump to dump 7.2 db -> 7.3 pg_restore. The bug reports I've seen were all such cases. Your test case seems 7.3 pg_dump to dump 7.2 db -> 7.3 pg_restore. Are you intending change my hack(? BLOB handling itself is a hack in PG) to solve both cases. regards, Hiroshi Inoue
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
>
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> >> That's one reason that I didn't like solving it by hacking pg_dump.
>
> > My fix works well with the scenario 7.2 pg_restore -> 7.3 pg_restore.
> > It's another problem that 7.3 pg_dump -> 7.3 pg_restore fails.
>
> Perhaps we're talking at cross-purposes. Exactly what was the failure
> that your fix was intended to prevent?
I've already mentioned it.
If you use 7.2 pg_dump to backup 7.2 db and use 7.3 pg_restore to
reload the data, you would see the failure
ERROR: Unable to identify an operator '=' for types 'oid' and 'lo'
You will have to retype this query using an explicit cast
7.3 pg_restore doesn't generate *CREATE CAST* commands
at all for 7.2 or earlier pg_dump data.
Usually people uses 7.2 pg_dump not 7.3 pg_dump to backup 7.2 db.
regards,
Hiroshi inoue
Hiroshi Inoue writes: > 7.3 pg_restore doesn't generate *CREATE CAST* commands > at all for 7.2 or earlier pg_dump data. > Usually people uses 7.2 pg_dump not 7.3 pg_dump to backup 7.2 db. If you propose to fix that problem you will have to add special cases for all data types in the world or find a general solution. -- Peter Eisentraut peter_e@gmx.net
Peter Eisentraut wrote:
>
> Hiroshi Inoue writes:
>
> > 7.3 pg_restore doesn't generate *CREATE CAST* commands
> > at all for 7.2 or earlier pg_dump data.
> > Usually people uses 7.2 pg_dump not 7.3 pg_dump to backup 7.2 db.
>
> If you propose to fix that problem you will have to add special
> cases for all data types in the world or find a general solution.
Cost effectiveness is very important even in open source
development. I believe I've spent much more time than I
should have spent on this trivial issue. I've just committed
my change to cvs. I would also commit it to 7.3-STABLE tree
(main target from the first) soon and end up this inneficient
discussion.
regards,
Hiroshi Inoue
http://w2422.nsk.ne.jp/~inoue/
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> ... I've just committed
> my change to cvs. I would also commit it to 7.3-STABLE tree
> (main target from the first) soon and end up this inneficient
> discussion.
When there are other people objecting to your proposal, trying
to force the issue by committing the change is not a good way to
proceed. Shall I voice my objections by reverting your changes?
I suggest discussing the issue instead of trying to force it.
regards, tom lane
Tom Lane wrote: > Hiroshi Inoue <Inoue@tpf.co.jp> writes: > > ... I've just committed > > my change to cvs. I would also commit it to 7.3-STABLE tree > > (main target from the first) soon and end up this inneficient > > discussion. > > When there are other people objecting to your proposal, trying > to force the issue by committing the change is not a good way to > proceed. Shall I voice my objections by reverting your changes? > > I suggest discussing the issue instead of trying to force it. Yes, I know discussion is a pain, but it is the cost of having others review our work. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian wrote:
>
> Tom Lane wrote:
> > Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > > ... I've just committed
> > > my change to cvs. I would also commit it to 7.3-STABLE tree
> > > (main target from the first) soon and end up this inneficient
> > > discussion.
> >
> > When there are other people objecting to your proposal, trying
> > to force the issue by committing the change is not a good way to
> > proceed. Shall I voice my objections by reverting your changes?
> >
> > I suggest discussing the issue instead of trying to force it.
>
> Yes, I know discussion is a pain, but it is the cost of having others
> review our work.
Unfortunately I don't have an infinite time and it's
about time I give up the discussion. Feel free to revert
my change. It's useless unless it is committed to 7.3 tree
ASAP. I have little time this week anyway.
regards,
Hiroshi Inoue
http://w2422.nsk.ne.jp/~inoue/
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Unfortunately I don't have an infinite time and it's
> about time I give up the discussion. Feel free to revert
> my change. It's useless unless it is committed to 7.3 tree
> ASAP. I have little time this week anyway.
I found that it's possible to modify the queries issued by FixupBlobRefs
so that they work with the declarations created from either 7.2 or 7.3
versions of contrib/lo. (Rather than assuming a cast exists, we can
write the function call lo(oid) or oid(lo) instead.)
This seems a much safer and more localized solution than having
pg_restore try to update the function definitions and create casts.
A related but independent problem is that 7.3 pg_dump tries to add
a CREATE CAST command when it sees a function that would have been
treated as a cast by earlier releases. The CREATE CAST command would
fail if the existing function was marked volatile --- which is the
default, and so quite likely to be the case for user-defined functions.
I've fixed this by removing the prohibition against volatile cast
functions (which was something that I was unhappy with from the first,
IIRC).
regards, tom lane
Tom Lane wrote:
>
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Unfortunately I don't have an infinite time and it's
> > about time I give up the discussion. Feel free to revert
> > my change. It's useless unless it is committed to 7.3 tree
> > ASAP. I have little time this week anyway.
>
> I found that it's possible to modify the queries issued by FixupBlobRefs
> so that they work with the declarations created from either 7.2 or 7.3
> versions of contrib/lo. (Rather than assuming a cast exists, we can
> write the function call lo(oid) or oid(lo) instead.)
>
> This seems a much safer and more localized solution than having
> pg_restore try to update the function definitions and create casts.
I don't object to your solution. I can't expect anything
but the waste of my time for the discussion.
It seems the stupid bug about tar format check was fixed.
But I still see a failure when I call 7.3 pg_dump for
a 7.3 database and call 7.3 pg_restore for the data.
I see the following in the log.
CREATE CAST (public.lo AS oid) WITH FUNCTION oid (public.lo);
ERROR: parser: parse error at or near "." at character 117
regards,
Hiroshi Inoue
http://www.geocities.jp/inocchichichi/psqlodbc/
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> CREATE CAST (public.lo AS oid) WITH FUNCTION oid (public.lo);
> ERROR: parser: parse error at or near "." at character 117
This turned out to be a lot easier to fix than I feared it would be.
There are nasty problems that show up if you try to generalize the
production for ConstTypename --- but there was no reason for CREATE/DROP
CAST to be using ConstTypename, they should have been using Typename.
Fix committed into HEAD and 7.3 branch.
regards, tom lane