Andres Freund <andres@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