pgsql-server/src/bin/pg_dump pg_backup_archiver.c

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

"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:

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

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