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

2014-08-14 15:11 GMT+02:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)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 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 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

Hi

2014-08-14 9:03 GMT+02:00 Heikki Linnakangas <hlinnakangas(at)vmware(dot)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.

> - Heikki
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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 15 August 2014 16:31, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> Hi
>
>
> 2014-08-14 9:03 GMT+02:00 Heikki Linnakangas <hlinnakangas(at)vmware(dot)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(dot)stehule(at)gmail(dot)com> wrote:
>
>> 2014-08-14 9:03 GMT+02:00 Heikki Linnakangas <hlinnakangas(at)vmware(dot)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

2014-09-30 17:18 GMT+02:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

>
> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>