Re: BUG #16655: pg_dump segfault when excluding postgis table

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
> > I'm getting a segfault when trying to pg_dump a database containing the
> > postgis extension, but only when excluding the spatial_ref_sys table.
>
> Yeah, I can reproduce this here.  You don't really need postgis.
> What I did was
>
> 1. Install the src/test/modules/test_pg_dump module into an
> empty database.
>
> 2. alter table regress_table_dumpable add check(col1 > 0);

While we certainly shouldn't just segfault, I don't think this is
actually something that's intended or should be supported- the extension
defines the table structure and users are really only expected to change
permissions (and related bits) on the table (and, to some extent, even
then only if they're familiar enough with the extension to know that the
extension understands that a user may change the privileges
post-install).

all-in-all, it's not really a great situation but it's at least better
than it had been (when we didn't try to deal with users changing
privileges on extension objects and people complained about that).

> 3. pg_dump -T regress_table_dumpable d1
>
> While I haven't traced through it in complete detail, what seems to
> be happening is
>
> A. checkExtensionMembership sees that the table belongs to an extension,
> hence marks it
>
>             dobj->dump = ext->dobj.dump_contains & (DUMP_COMPONENT_ACL |
>                                                     DUMP_COMPONENT_SECLABEL |
>                                                     DUMP_COMPONENT_POLICY);
>
> (This ends up setting only the SECLABEL and POLICY bits, I didn't
> check why.)
>
> B. processExtensionTables sees that the table is excluded by an exclusion
> switch, so it sets configtbl->interesting = false.
>
> (I've not verified whether B happens before or after A.  But we definitely
> end up with a TableInfo having dobj.dump = 40, interesting = false.)
>
> C. getTableAttrs sees that the table is marked interesting = false,
> so it doesn't bother loading any subsidiary data; particularly not
> the checkexprs[] array.  But ncheck is positive because getTables filled
> it.
>
> D. Since dobj.dump is non-zero, we eventually wind up at dumpTableSchema,
> which crashes because checkexprs is NULL.  It's a bit surprising that
> it doesn't crash sooner, but whatever.  We should absolutely not be in
> that code at all for a table we didn't load all the subsidiary data for.
>
> So I think the basic problem here is that checkExtensionMembership and
> processExtensionTables are not on the same page.  We can't have
> interesting = false for a table that any of the dobj.dump bits are set
> for.
>
> Arguably, we should get rid of the "interesting" flag entirely now that
> we have dobj.dump.  I can't see that it's anything but a foot-gun.
>
> Stephen, I think you touched this code last; any thoughts?

I had contemplated trying to get rid of the 'interesting' flag but, my
recollection anyway, is that it needed to be set sometimes even though
dobj.dump wasn't.  Has been a number of years though and either my
memory or my review at the time might be faulty.

I do agree with the general idea of trying to get rid of it though, and
instead using dobj.dump to decide when we need to load additional info.

In the end though, with this case at least, either way, we don't
currently track what constraints are installed by the extension and what
are installed by users after, and so we can't possibly recreate the
object in the same way that the user has- if we include all CHECK
constraints then we'll end up with duplicates for any that the extension
created originally, and if we don't include any then the restored table
will be missing any the user added.

Ultimately, if getting rid of 'interesting' and replacing it with
checking dobj.dump works, great, but that isn't going to actually make
this case 'work' for the user, it'll just make pg_dump not crash (which
is certainly good) but the added CHECK constraint will get lost.

Thinking about it a bit more, I'd be inclined to argue that what we
should really do here is mark tables created by extensions in a similar
manner that system tables are- basically, if you try to ALTER them, you
get an error unless you've set some extra bit that says you know what
you're doing and that you understand your change will get lost if you
dump/reload or pg_upgrade.

Thanks,

Stephen