Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes: > There is a lot of version dependent code in pg_dump now, especially > per-version queries. The way they were originally built was that we > just have an entirely separate query per version. But as the number of > versions and the size of the query grows, this becomes unwieldy.
Agreed, it's becoming a bit of a mess. > So I tried, as an example, to write the queries in getTableAttrs() > differently. Instead of repeating the almost same large query in each > version branch, use one query and add a few columns to the SELECT list > depending on the version. This saves a lot of duplication and is easier > to deal with. I think I had this discussion already with somebody, but ... I do not like this style at all: tbinfo->attidentity[j] = (i_attidentity >= 0 ? *(PQgetvalue(res, j, i_attidentity)) : '\0'); It's basically throwing away all the cross-checking that exists now to ensure that you didn't forget to deal with a column, misspell the column's AS name in one place or the other, etc etc. The code could be completely wrong and it'd still fail silently, producing a default result that might well be the right answer, making it hard to debug. I think the way to do this is to continue to require the query to produce the same set of columns regardless of server version. So the version-dependent bits would look like, eg, if (fout->remoteVersion >= 110000) appendPQExpBufferStr(q, "CASE WHEN a.atthasmissing AND NOT a.attisdropped " "THEN a.attmissingval ELSE null END AS attmissingval,\n"); else appendPQExpBufferStr(q, "null AS attmissingval,\n"); and the data extraction code would remain the same as now. Other than that nit, I agree that this shows a lot of promise. regards, tom lane