Re: some more pg_dump refactoring
Fabien COELHO <coelho@cri.ensmp.fr>
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2020-06-25T06:58:58Z
Lists: pgsql-hackers
Hallo Peter, My 0.02 €: Patch applies cleanly, compiles, make check and pg_dump tap tests ok. The refactoring is a definite improvements. You changed the query strings to use "\n" instead of " ". I would not have changed that, because it departs from the style around, and I do not think it improves readability at the C code level. I tried to check manually and randomly that the same query is built for the same version, although my check may have been partial, especially on the aggregate query which does not comment about what is changed between versions, and my eyes are not very good at diffing. I've notice that some attributes are given systematic replacements (eg proparallel), removing the need to test for presence afterwards. This looks fine to me. However, on version < 8.4, ISTM that funcargs and funciargs are always added: is this voluntary?. Would it make sense to accumulate in the other direction, older to newer, so that new attributes are added at the end of the select? Should array_to_string be pg_catalog.array_to_string? All other calls seem to have an explicit schema. I'm fine with inlining most PQfnumber calls. I do not have old versions at hand for testing. > Here is a patch to reorganize dumpFunc() and dumpAgg() in pg_dump, similar to > daa9fe8a5264a3f192efa5ddee8fb011ad9da365. 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. > > I have tested this with various old versions of PostgreSQL I had available, > but a bit more random testing with old versions would be welcome. -- Fabien.
Commits
-
pg_dump: Reorganize dumpFunc() and dumpAgg()
- ed2c7f65bd9f 14.0 landed
-
pg_dump: Further reorganize getTableAttrs()
- 991c444e7ad3 14.0 landed
-
pg_dump: Reorganize getTableAttrs()
- daa9fe8a5264 12.0 cited