Обсуждение: 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)

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

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

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

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

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

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

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