Andres Freund <and...@anarazel.de> writes: > On 2019-05-22 14:17:41 -0400, Tom Lane wrote: >> 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. No, the concerns I have are about (1) failure to insert the extra return column into all branches where it's needed; (2) some unexpected run-time problem causing the PGresult to not have the expected column. I think we've had some discussions about restructuring those giant if-nests so that they build up the query in pieces, making it possible to write things along the lines of appendPQExpBuffer(query, "SELECT c.tableoid, c.oid, c.relname, " // version-independent stuff here ...); ... if (fout->remoteVersion >= 120000) appendPQExpBuffer(query, "am.amname, "); else appendPQExpBuffer(query, "NULL as amname, "); ... I'm not sure if it'd work quite that cleanly when we need changes in the FROM part, but certainly for newly-added result fields this would be hugely better than repeating the whole query. And yes, I'd still insist that explicitly providing the alternative NULL value is not optional. >> 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? Oh, I see: int PQgetisnull(const PGresult *res, int tup_num, int field_num) { if (!check_tuple_field_number(res, tup_num, field_num)) return 1; /* pretend it is null */ which just happens to be the right thing --- in this case --- for the back branches. regards, tom lane