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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> 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.

Yes, I had misunderstood what the use-case being discussed here was,
having focused on just the stripped-down test case you were using.

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

I agree we shouldn't be crashing in such a case.

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> BTW, the reason we can't get rid of the "interesting" flag is that
> we need to set it on parent tables of tables we need to dump,
> even if we're not going to dump the parents themselves (hence their
> dobj.dump is zero).  Otherwise we won't collect the subsidiary data
> we need to figure out which parts of the child's definition are
> inherited.

Ah, yes, that matches my recollection.

Should we ever drop support for inheritance, perhaps then we could be
rid of it.

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I wrote:
> > It's not entirely clear to me why we don't get a crash on the case in
> > pre-v12 branches, but I'm inclined to apply these patches all the way
> > back anyway, as they definitely seem like robustness improvements
> > whether or not a given branch accidentally fails to fail.
>
> OK, so after a bit of "git bisect"ing, things are clearer:
>
> * Before 3eb3d3e78, there was no code path (or at least no known one)
> that would allow control to arrive at dumpTableSchema without having
> done getTableAttrs.  So my original thought that that was a logical
> prerequisite was correct.
>
> * Before v12, dumpTableData_insert would kind-of-accidentally work
> without having done getTableAttrs, because it got all the needed
> info from the PGresult of its "SELECT * FROM ONLY table" query,
> rather than looking at the per-column data in the TableInfo.
> v12 added a check on the attgenerated[] data, causing that to not
> work anymore, which was the reported bug leading to 3eb3d3e78.
>
> * However, the dumpTableData_copy code path also was only
> accidentally working without getTableAttrs.  That left numattrs
> zero in the TableInfo, causing fmtCopyColumnList to return an
> empty string, which works almost all the time for both the
> data fetching and the eventual reload.  It'd only fail obviously
> in corner cases where the column order needed to be different at
> reload; so it's not so surprising that we'd not noticed.
> Nonetheless, *both* the COPY and INSERT code paths are dependent
> on getTableAttrs, contrary to the discussion in the other thread;
> and the COPY case's dependency goes back further.
>
> I conclude that:
>
> * My 0003 is the actually important fix.  0001 just adds some
> robustness, which is not a bad thing but it doesn't seem critical.
>
> * I should revise 0002 so that it exercises the COPY path and
> checks that the correct column list is given.  I expect this will
> show that there's a problem further back than v12.
>
> * We should add Asserts in both dumpTableData and dumpTableSchema
> to verify that "interesting" is set, in hopes of catching any
> future mistakes of this ilk.

This all sounds like a reasonable approach to me.  I've gone back and
looked through things a bit and agree that processExtensionTables really
should be setting interesting to true for extension config tables when
we decide we want to include them.  Your 0003 patch looks correct to me,
and it does seem like we need to go all the way back with that.

Thanks,

Stephen