Re: some pg_dump query code simplification

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)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