Re: some pg_dump query code simplification

Peter Eisentraut <peter.eisentraut@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