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: <2007400.1602019564@sss.pgh.pa.us>
References: <16655-5c92d6b3a9438137@postgresql.org>
 <1819571.1601937068@sss.pgh.pa.us>
 <20201006163318.GX3063@tamriel.snowman.net>
 <1909344.1602003707@sss.pgh.pa.us> <1995213.1602011218@sss.pgh.pa.us>
 <2007400.1602019564@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 06 Oct 2020 17:26:04 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2037840.1602032684.1@sss.pgh.pa.us>
Date: Tue, 06 Oct 2020 21:04:44 -0400
Message-ID: <2037841.1602032684@sss.pgh.pa.us>
Precedence: bulk

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.

			regards, tom lane