From: Tom Lane <tgl@sss.pgh.pa.us>
To: Stephen Frost <sfrost@snowman.net>
cc: cam.daniel@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16655: pg_dump segfault when excluding postgis table
In-reply-to: <20201006163318.GX3063@tamriel.snowman.net>
References: <16655-5c92d6b3a9438137@postgresql.org>
 <1819571.1601937068@sss.pgh.pa.us>
 <20201006163318.GX3063@tamriel.snowman.net>
Comments: In-reply-to Stephen Frost <sfrost@snowman.net>
	message dated "Tue, 06 Oct 2020 12:33:18 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1909343.1602003707.1@sss.pgh.pa.us>
Date: Tue, 06 Oct 2020 13:01:47 -0400
Message-ID: <1909344.1602003707@sss.pgh.pa.us>
Precedence: bulk

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> 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).

I think you misunderstand my point here.  pg_dump would still segfault
if the CHECK constraint had been created by the extension (which, indeed,
is what I'm thinking of doing to convert this into a regression test).
Presumably, the reason it's failing on postgis is that spatial_ref_sys
has some extension-defined CHECK constraints.

I'm not intending to suggest that pg_dump ought to understand this
situation enough to dump the CHECK constraint --- I'm just describing a
quick way of reproducing the crash, without having to install postgis.
I think the actual case of interest is "-T extension_table should not
result in a crash when extension_table has CHECK constraints".

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

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

OK, I'll study this some more.

			regards, tom lane