pg_dump bug in 9.4beta2 and HEAD
I'm seeing an assertion failure with "pg_dump -c --if-exists" which is
not ready to handle BLOBs it seems:
pg_dump: pg_backup_archiver.c:472: RestoreArchive: Assertion `mark !=
((void *)0)' failed.
To reproduce:
$ createdb test
$ pg_dump -c --if-exists test (works, dumps empty database)
$ psql test -c "select lo_create(1);"
$ pg_dump -c --if-exists test (fails, with the above mentioned assertion)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/14/2014 06:53 AM, Joachim Wieland wrote:
I'm seeing an assertion failure with "pg_dump -c --if-exists" which is
not ready to handle BLOBs it seems:pg_dump: pg_backup_archiver.c:472: RestoreArchive: Assertion `mark !=
((void *)0)' failed.To reproduce:
$ createdb test
$ pg_dump -c --if-exists test (works, dumps empty database)
$ psql test -c "select lo_create(1);"
$ pg_dump -c --if-exists test (fails, with the above mentioned assertion)
The code tries to inject an "IF EXISTS" into the already-construct DROP
command, but it doesn't work for large objects, because the deletion
command looks like "SELECT pg_catalog.lo_unlink(xxx)". There is no DROP
there.
I believe we could use "SELECT pg_catalog.lo_unlink(loid) FROM
pg_catalog.pg_largeobject_metadata WHERE loid = xxx".
pg_largeobject_metadata table didn't exist before version 9.0, but we
don't guarantee pg_dump's output to be compatible in that direction
anyway, so I think that's fine.
The quick fix would be to add an exception for blobs, close to where
Assert is. There are a few exceptions there already. A cleaner solution
would be to add a new argument to ArchiveEntry and make the callers
responsible for providing an "DROP IF EXISTS" query, but that's not too
appetizing because for most callers it would be boring boilerplate code.
Perhaps add an argument, but if it's NULL, ArchiveEntry would form the
if-exists query automatically from the DROP query.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas wrote:
On 08/14/2014 06:53 AM, Joachim Wieland wrote:
I'm seeing an assertion failure with "pg_dump -c --if-exists" which is
not ready to handle BLOBs it seems:pg_dump: pg_backup_archiver.c:472: RestoreArchive: Assertion `mark !=
((void *)0)' failed.To reproduce:
$ createdb test
$ pg_dump -c --if-exists test (works, dumps empty database)
$ psql test -c "select lo_create(1);"
$ pg_dump -c --if-exists test (fails, with the above mentioned assertion)The code tries to inject an "IF EXISTS" into the already-construct
DROP command, but it doesn't work for large objects, because the
deletion command looks like "SELECT pg_catalog.lo_unlink(xxx)".
There is no DROP there.
Ah, so this was broken by 9067310cc5dd590e36c2c3219dbf3961d7c9f8cb.
Pavel, any thoughts here? Can you provide a fix?
We already have a couple of object-type-specific checks in there, so I
think it's okay to add one more exception for large objects.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2014-08-14 15:11 GMT+02:00 Alvaro Herrera <alvherre@2ndquadrant.com>:
Heikki Linnakangas wrote:
On 08/14/2014 06:53 AM, Joachim Wieland wrote:
I'm seeing an assertion failure with "pg_dump -c --if-exists" which is
not ready to handle BLOBs it seems:pg_dump: pg_backup_archiver.c:472: RestoreArchive: Assertion `mark !=
((void *)0)' failed.To reproduce:
$ createdb test
$ pg_dump -c --if-exists test (works, dumps empty database)
$ psql test -c "select lo_create(1);"
$ pg_dump -c --if-exists test (fails, with the above mentionedassertion)
The code tries to inject an "IF EXISTS" into the already-construct
DROP command, but it doesn't work for large objects, because the
deletion command looks like "SELECT pg_catalog.lo_unlink(xxx)".
There is no DROP there.Ah, so this was broken by 9067310cc5dd590e36c2c3219dbf3961d7c9f8cb.
Pavel, any thoughts here? Can you provide a fix?We already have a couple of object-type-specific checks in there, so I
think it's okay to add one more exception for large objects.
i will prepare this fix today
regards
Pavel
Show quoted text
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Heikki Linnakangas wrote:
The quick fix would be to add an exception for blobs, close to where
Assert is. There are a few exceptions there already. A cleaner
solution would be to add a new argument to ArchiveEntry and make the
callers responsible for providing an "DROP IF EXISTS" query, but
that's not too appetizing because for most callers it would be
boring boilerplate code. Perhaps add an argument, but if it's NULL,
ArchiveEntry would form the if-exists query automatically from the
DROP query.
Yeah, this was discussed (or at least mentioned) in the original thread.
See
/messages/by-id/20140228183100.GU4759@eldon.alvh.no-ip.org
where I wrote:
: I still find the code to inject IF EXISTS to the DROP commands ugly as
: sin. I would propose to stop storing the dropStmt in the archive
: anymore; instead just store the object identity, which can later be used
: to generate both DROP commands, with or without IF EXISTS, and the ALTER
: OWNER commands. However, that's a larger project and I don't think we
: need to burden this patch with that.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi
2014-08-14 9:03 GMT+02:00 Heikki Linnakangas <hlinnakangas@vmware.com>:
On 08/14/2014 06:53 AM, Joachim Wieland wrote:
I'm seeing an assertion failure with "pg_dump -c --if-exists" which is
not ready to handle BLOBs it seems:pg_dump: pg_backup_archiver.c:472: RestoreArchive: Assertion `mark !=
((void *)0)' failed.To reproduce:
$ createdb test
$ pg_dump -c --if-exists test (works, dumps empty database)
$ psql test -c "select lo_create(1);"
$ pg_dump -c --if-exists test (fails, with the above mentioned assertion)The code tries to inject an "IF EXISTS" into the already-construct DROP
command, but it doesn't work for large objects, because the deletion
command looks like "SELECT pg_catalog.lo_unlink(xxx)". There is no DROP
there.I believe we could use "SELECT pg_catalog.lo_unlink(loid) FROM
pg_catalog.pg_largeobject_metadata WHERE loid = xxx".
pg_largeobject_metadata table didn't exist before version 9.0, but we don't
guarantee pg_dump's output to be compatible in that direction anyway, so I
think that's fine.The quick fix would be to add an exception for blobs, close to where
Assert is. There are a few exceptions there already. A cleaner solution
would be to add a new argument to ArchiveEntry and make the callers
responsible for providing an "DROP IF EXISTS" query, but that's not too
appetizing because for most callers it would be boring boilerplate code.
Perhaps add an argument, but if it's NULL, ArchiveEntry would form the
if-exists query automatically from the DROP query.
I am sending two patches
first is fast fix
second fix is implementation of Heikki' idea.
Show quoted text
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
lo-if-exists-heikki.patchtext/x-patch; charset=US-ASCII; name=lo-if-exists-heikki.patchDownload
commit 68ba9d3b682c6e56b08d623ebc58e351ce1a6c55
Author: Pavel Stehule <pavel.stehule@gooddata.com>
Date: Fri Aug 15 13:41:24 2014 +0200
heikki-lo-if-exists-fix
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 3aebac8..0049be7 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -429,60 +429,100 @@ RestoreArchive(Archive *AHX)
}
else
{
- char buffer[40];
- char *mark;
- char *dropStmt = pg_strdup(te->dropStmt);
- char *dropStmtPtr = dropStmt;
- PQExpBuffer ftStmt = createPQExpBuffer();
-
/*
- * Need to inject IF EXISTS clause after ALTER TABLE
- * part in ALTER TABLE .. DROP statement
+ * LO doesn't use DDL stmts, instead it uses lo_unlink
+ * functions. But it should be injected too.
*/
- if (strncmp(dropStmt, "ALTER TABLE", 11) == 0)
+ if (strncmp(te->desc, "BLOB", 4) == 0)
{
- appendPQExpBuffer(ftStmt,
- "ALTER TABLE IF EXISTS");
- dropStmt = dropStmt + 11;
+ Oid loid = InvalidOid;
+ char *mark;
+
+ /* find and take foid */
+ if (strncmp(te->dropStmt, "SELECT pg_catalog.lo_unlink('", 29) == 0)
+ {
+ unsigned long cvt;
+ char *endptr;
+
+ mark = te->dropStmt + 29;
+
+ if (*mark != '\0')
+ {
+ errno = 0;
+ cvt = strtoul(mark, &endptr, 10);
+
+ /* Ensure we have a valid Oid */
+ if (errno == 0 && mark != endptr && *endptr == '\'')
+ loid = (Oid) cvt;
+ }
+ }
+
+ if (loid == InvalidOid)
+ exit_horribly(modulename, "cannot to identify oid of large object\n");
+
+ /*
+ * Now we have valid foid and we can generate
+ * fault tolerant (not existency) SQL statement.
+ */
+ DropBlobIfExists(AH, loid);
}
-
- /*
- * ALTER TABLE..ALTER COLUMN..DROP DEFAULT does not
- * support the IF EXISTS clause, and therefore we
- * simply emit the original command for such objects.
- * For other objects, we need to extract the first
- * part of the DROP which includes the object type.
- * Most of the time this matches te->desc, so search
- * for that; however for the different kinds of
- * CONSTRAINTs, we know to search for hardcoded "DROP
- * CONSTRAINT" instead.
- */
- if (strcmp(te->desc, "DEFAULT") == 0)
- appendPQExpBuffer(ftStmt, "%s", dropStmt);
else
{
- if (strcmp(te->desc, "CONSTRAINT") == 0 ||
- strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
- strcmp(te->desc, "FK CONSTRAINT") == 0)
- strcpy(buffer, "DROP CONSTRAINT");
+ char buffer[40];
+ char *mark;
+ char *dropStmt = pg_strdup(te->dropStmt);
+ char *dropStmtPtr = dropStmt;
+ PQExpBuffer ftStmt = createPQExpBuffer();
+
+ /*
+ * Need to inject IF EXISTS clause after ALTER TABLE
+ * part in ALTER TABLE .. DROP statement
+ */
+ if (strncmp(dropStmt, "ALTER TABLE", 11) == 0)
+ {
+ appendPQExpBuffer(ftStmt,
+ "ALTER TABLE IF EXISTS");
+ dropStmt = dropStmt + 11;
+ }
+
+ /*
+ * ALTER TABLE..ALTER COLUMN..DROP DEFAULT does not
+ * support the IF EXISTS clause, and therefore we
+ * simply emit the original command for such objects.
+ * For other objects, we need to extract the first
+ * part of the DROP which includes the object type.
+ * Most of the time this matches te->desc, so search
+ * for that; however for the different kinds of
+ * CONSTRAINTs, we know to search for hardcoded "DROP
+ * CONSTRAINT" instead.
+ */
+ if (strcmp(te->desc, "DEFAULT") == 0)
+ appendPQExpBuffer(ftStmt, "%s", dropStmt);
else
- snprintf(buffer, sizeof(buffer), "DROP %s",
- te->desc);
+ {
+ if (strcmp(te->desc, "CONSTRAINT") == 0 ||
+ strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
+ strcmp(te->desc, "FK CONSTRAINT") == 0)
+ strcpy(buffer, "DROP CONSTRAINT");
+ else
+ snprintf(buffer, sizeof(buffer), "DROP %s",
+ te->desc);
- mark = strstr(dropStmt, buffer);
- Assert(mark != NULL);
+ mark = strstr(dropStmt, buffer);
+ Assert(mark != NULL);
- *mark = '\0';
- appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
- dropStmt, buffer,
- mark + strlen(buffer));
- }
+ *mark = '\0';
+ appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
+ dropStmt, buffer,
+ mark + strlen(buffer));
+ }
- ahprintf(AH, "%s", ftStmt->data);
+ ahprintf(AH, "%s", ftStmt->data);
- destroyPQExpBuffer(ftStmt);
+ destroyPQExpBuffer(ftStmt);
- pg_free(dropStmtPtr);
+ pg_free(dropStmtPtr);
+ }
}
}
}
simple-fix.patchtext/x-patch; charset=US-ASCII; name=simple-fix.patchDownload
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
new file mode 100644
index 3aebac8..3c2ee84
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*************** RestoreArchive(Archive *AHX)
*** 435,440 ****
--- 435,443 ----
char *dropStmtPtr = dropStmt;
PQExpBuffer ftStmt = createPQExpBuffer();
+ if (strncmp(te->desc, "BLOB", 4) == 0)
+ exit_horribly(modulename, "large objects doesn't support --if-exists option\n");
+
/*
* Need to inject IF EXISTS clause after ALTER TABLE
* part in ALTER TABLE .. DROP statement
On Thu, Aug 14, 2014 at 10:03:57AM +0300, Heikki Linnakangas wrote:
On 08/14/2014 06:53 AM, Joachim Wieland wrote:
I'm seeing an assertion failure with "pg_dump -c --if-exists" which is
not ready to handle BLOBs it seems:pg_dump: pg_backup_archiver.c:472: RestoreArchive: Assertion `mark !=
((void *)0)' failed.To reproduce:
$ createdb test
$ pg_dump -c --if-exists test (works, dumps empty database)
$ psql test -c "select lo_create(1);"
$ pg_dump -c --if-exists test (fails, with the above mentioned assertion)The code tries to inject an "IF EXISTS" into the already-construct DROP
command, but it doesn't work for large objects, because the deletion command
looks like "SELECT pg_catalog.lo_unlink(xxx)". There is no DROP there.
The lo_* functions are probably too entrenched to be deprecated, but
maybe we could come up with DML (or DDL, although that seems like a
bridge too far) equivalents and use those. Not for 9.4, obviously.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 15 August 2014 16:31, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hi
2014-08-14 9:03 GMT+02:00 Heikki Linnakangas <hlinnakangas@vmware.com>:
On 08/14/2014 06:53 AM, Joachim Wieland wrote:
I'm seeing an assertion failure with "pg_dump -c --if-exists" which is
not ready to handle BLOBs it seems:pg_dump: pg_backup_archiver.c:472: RestoreArchive: Assertion `mark !=
((void *)0)' failed.To reproduce:
$ createdb test
$ pg_dump -c --if-exists test (works, dumps empty database)
$ psql test -c "select lo_create(1);"
$ pg_dump -c --if-exists test (fails, with the above mentioned
assertion)The code tries to inject an "IF EXISTS" into the already-construct DROP
command, but it doesn't work for large objects, because the deletion
command looks like "SELECT pg_catalog.lo_unlink(xxx)". There is no DROP
there.I believe we could use "SELECT pg_catalog.lo_unlink(loid) FROM
pg_catalog.pg_largeobject_metadata WHERE loid = xxx".
pg_largeobject_metadata table didn't exist before version 9.0, but we don't
guarantee pg_dump's output to be compatible in that direction anyway, so I
think that's fine.The quick fix would be to add an exception for blobs, close to where
Assert is. There are a few exceptions there already. A cleaner solution
would be to add a new argument to ArchiveEntry and make the callers
responsible for providing an "DROP IF EXISTS" query, but that's not too
appetizing because for most callers it would be boring boilerplate code.
Perhaps add an argument, but if it's NULL, ArchiveEntry would form the
if-exists query automatically from the DROP query.I am sending two patches
first is fast fix
second fix is implementation of Heikki' idea.
I'm guessing this issue is still unresolved? It would be nice to get this
off the open items list.
Thom
On 09/24/2014 01:50 PM, Thom Brown wrote:
On 15 August 2014 16:31, Pavel Stehule <pavel.stehule@gmail.com> wrote:
2014-08-14 9:03 GMT+02:00 Heikki Linnakangas <hlinnakangas@vmware.com>:
On 08/14/2014 06:53 AM, Joachim Wieland wrote:
I'm seeing an assertion failure with "pg_dump -c --if-exists" which is
not ready to handle BLOBs it seems:pg_dump: pg_backup_archiver.c:472: RestoreArchive: Assertion `mark !=
((void *)0)' failed.To reproduce:
$ createdb test
$ pg_dump -c --if-exists test (works, dumps empty database)
$ psql test -c "select lo_create(1);"
$ pg_dump -c --if-exists test (fails, with the above mentioned
assertion)The code tries to inject an "IF EXISTS" into the already-construct DROP
command, but it doesn't work for large objects, because the deletion
command looks like "SELECT pg_catalog.lo_unlink(xxx)". There is no DROP
there.I believe we could use "SELECT pg_catalog.lo_unlink(loid) FROM
pg_catalog.pg_largeobject_metadata WHERE loid = xxx".
pg_largeobject_metadata table didn't exist before version 9.0, but we don't
guarantee pg_dump's output to be compatible in that direction anyway, so I
think that's fine.The quick fix would be to add an exception for blobs, close to where
Assert is. There are a few exceptions there already. A cleaner solution
would be to add a new argument to ArchiveEntry and make the callers
responsible for providing an "DROP IF EXISTS" query, but that's not too
appetizing because for most callers it would be boring boilerplate code.
Perhaps add an argument, but if it's NULL, ArchiveEntry would form the
if-exists query automatically from the DROP query.I am sending two patches
first is fast fix
second fix is implementation of Heikki' idea.
I'm guessing this issue is still unresolved? It would be nice to get this
off the open items list.
Yeah, I had completely forgotten about this. Alvaro, could you finish
this off?
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas wrote:
On 09/24/2014 01:50 PM, Thom Brown wrote:
I am sending two patches
first is fast fix
second fix is implementation of Heikki' idea.
I'm guessing this issue is still unresolved? It would be nice to get this
off the open items list.Yeah, I had completely forgotten about this. Alvaro, could you
finish this off?
Ah, I had forgotten too. Sure, will look into it shortly.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I have pushed this fix, except that instead of parsing the OID from the
dropStmt as in your patch, I used te->catalogId.oid, which is much
simpler.
I tested this by pg_restoring to 8.4 (which doesn't have
pg_largeobject_metadata); there is no error raised:
LOG: sentencia: SELECT CASE WHEN EXISTS(SELECT 1 FROM pg_catalog.pg_largeobject WHERE loid = '43748') THEN pg_catalog.lo_unlink('43748') END;
In 9.0 the command is the new style:
LOG: sentencia: SELECT pg_catalog.lo_unlink(oid) FROM pg_catalog.pg_largeobject_metadata WHERE oid = '43748';
So it's all fine. I guess it's fortunate that we already had the
DropBlobIfExists() function.
Now a further problem I notice is all the *other* commands for which we
inject the IF EXISTS clause; there is no support for those in older
servers, so they throw errors if --if-exists is given in the pg_restore
line. I think we can just say that --if-exists is not supported for
older servers; as Heikki said, we don't promise that pg_dump is
compatible with older servers anyway. In my test database, several
commands errored out when seeing the EXTENSION in CREATE EXTENSION IF EXISTS.
So we're okay now.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2014-09-30 17:18 GMT+02:00 Alvaro Herrera <alvherre@2ndquadrant.com>:
I have pushed this fix, except that instead of parsing the OID from the
dropStmt as in your patch, I used te->catalogId.oid, which is much
simpler.
yes, it is much better
I tested this by pg_restoring to 8.4 (which doesn't have
pg_largeobject_metadata); there is no error raised:LOG: sentencia: SELECT CASE WHEN EXISTS(SELECT 1 FROM
pg_catalog.pg_largeobject WHERE loid = '43748') THEN
pg_catalog.lo_unlink('43748') END;In 9.0 the command is the new style:
LOG: sentencia: SELECT pg_catalog.lo_unlink(oid) FROM
pg_catalog.pg_largeobject_metadata WHERE oid = '43748';So it's all fine. I guess it's fortunate that we already had the
DropBlobIfExists() function.Now a further problem I notice is all the *other* commands for which we
inject the IF EXISTS clause; there is no support for those in older
servers, so they throw errors if --if-exists is given in the pg_restore
line. I think we can just say that --if-exists is not supported for
older servers; as Heikki said, we don't promise that pg_dump is
compatible with older servers anyway. In my test database, several
commands errored out when seeing the EXTENSION in CREATE EXTENSION IF
EXISTS.
So we're okay now.
great,
Thank you very much
Pavel
Show quoted text
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services