Hi, On 2019-05-22 14:17:41 -0400, Tom Lane wrote: > Andres Freund <and...@anarazel.de> writes: > > Wouldn't the better fix be to change > > if (PQgetisnull(res, i, i_amname)) > > tblinfo[i].amname = NULL; > > into > > if (i_amname == -1 || PQgetisnull(res, i, i_amname)) > > tblinfo[i].amname = NULL; > > it's much more scalable than adding useless columns everywhere, and we > > already use that approach with i_checkoption (and at a number of other > > places). > > FWIW, I think that's a pretty awful idea, and the fact that some > people have had it before doesn't make it less awful. It's giving > up the ability to detect errors-of-omission, which might easily > be harmful rather than harmless errors.
Well, if we explicitly have to check for -1, it's not really an error of omission for everything. Yes, we could forget returning the amname in a newer version of the query, but given that we usually just forward copy the query that's not that likely. And instead having to change a lot of per-branch queries also adds potential for error, and also makes it more painful to fix cross-branch bugs etc. > >> Looks like the right fix. I'm sad that the buildfarm did not catch > >> this ... why wouldn't the cross-version-upgrade tests have seen it? > > > I suspect we just didn't notice that it saw that: > > as it's just a notice, not a failure. > > Hm. But shouldn't we have gotten garbage output from the pg_dump run? What garbage? We'd take the column as NULL, and assume it doesn't have an assigned AM. Which is the right behaviour when dumping from < 12? Greetings, Andres Freund
