From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
---|---|
To: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
Cc: | Surafel Temesgen <surafel3000(at)gmail(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pg_dump multi VALUES INSERT |
Date: | 2019-02-02 08:26:08 |
Message-ID: | alpine.DEB.2.21.1902011635430.26597@lancre |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello David,
> Wondering if you have anything else here? I'm happy for the v13
> version to be marked as ready for committer.
I still have a few comments.
Patch applies cleanly, compiles, global & local make check are ok.
Typos and style in the doc:
"However, since, by default this option generates ..."
"However, since this option, by default, generates ..."
I'd suggest a more straightforward to my mind and ear: "However, since by
default the option generates ..., ....", although beware that I'm not a
native English speaker.
I do not understand why dump_inserts declaration has left the "flags for
options" section.
I'd suggest not to rely on "atoi" because it does not check the argument
syntax, so basically anything is accepted, eg "1O" is 1;
On "if (!dopt->do_nothing) $1 else $2;", I'd rather use a straight
condition "if (dopt->do_nothing) $2 else $1;" (two instances).
There is a test, that is good! Charater "." should be backslashed in the
regexpr. I'd consider also introducing limit cases: empty table, empty
columns by creating corresponding tables and using -t repeatedly.
--
Fabien.
From | Date | Subject | |
---|---|---|---|
Next Message | Noah Misch | 2019-02-02 08:38:22 | Spurious "apparent wraparound" via SimpleLruTruncate() rounding |
Previous Message | Noah Misch | 2019-02-02 08:18:33 | Re: pgsql: Remove references to Majordomo |