Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
2014-09-30 17:18 GMT+02:00 Alvaro Herrera :
>
> 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
>
> --
> Álvaro Herrerahttp://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
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 Herrerahttp://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
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
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 Herrerahttp://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
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
On 09/24/2014 01:50 PM, Thom Brown wrote: On 15 August 2014 16:31, Pavel Stehule wrote: 2014-08-14 9:03 GMT+02:00 Heikki Linnakangas : 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
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
On 15 August 2014 16:31, Pavel Stehule wrote: > Hi > > > 2014-08-14 9:03 GMT+02:00 Heikki Linnakangas : > >> 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
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
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 http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@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
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
Hi
2014-08-14 9:03 GMT+02:00 Heikki Linnakangas :
> 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.
> - Heikki
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
commit 68ba9d3b682c6e56b08d623ebc58e351ce1a6c55
Author: Pavel Stehule
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)
+ {
+
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
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 http://www.postgresql.org/message-id/20140228183100.gu4...@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 Herrerahttp://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
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
2014-08-14 15:11 GMT+02:00 Alvaro Herrera : > 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. > i will prepare this fix today regards Pavel > > -- > Álvaro Herrerahttp://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services >
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
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 Herrerahttp://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
Re: [HACKERS] pg_dump bug in 9.4beta2 and HEAD
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
