From: Tom Lane <tgl@sss.pgh.pa.us>
To: cam.daniel@gmail.com
cc: pgsql-bugs@lists.postgresql.org, Stephen Frost <sfrost@snowman.net>
Subject: Re: BUG #16655: pg_dump segfault when excluding postgis table
In-reply-to: <16655-5c92d6b3a9438137@postgresql.org>
References: <16655-5c92d6b3a9438137@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Mon, 05 Oct 2020 20:54:43 -0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1819570.1601937068.1@sss.pgh.pa.us>
Date: Mon, 05 Oct 2020 18:31:08 -0400
Message-ID: <1819571.1601937068@sss.pgh.pa.us>
Precedence: bulk

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);

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?

			regards, tom lane