Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> 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.
+1 to Tom's thoughts on this- seems like a much better approach.
Overall, I definitely agree that it'd be nice to reduce the amount of
duplication happening today. Working out some way to actually test all
of those queries would be awful nice too.
I wonder- what if we had an option to pg_dump to explicitly tell it what
the server's version is and then have TAP tests to run with different
versions? There may be some cases where that doesn't work, of course,
and we'd need to address that, but having those tests might reduce the
number of times we end up with something committed which doesn't work at
all against some older version of PG because it wasn't tested...
Thanks!
Stephen