Hi,
On 2019-05-22 14:17:41 -0400, Tom Lane wrote:
> Andres Freund <andres@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