Re: pg_dump multi VALUES INSERT

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.

In response to

Responses

Browse pgsql-hackers by date

  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