Re: pg_dump multi VALUES INSERT - Mailing list pgsql-hackers

From Fabien COELHO
Subject Re: pg_dump multi VALUES INSERT
Date
Msg-id alpine.DEB.2.21.1902011635430.26597@lancre
Whole thread Raw
In response to Re: pg_dump multi VALUES INSERT  (David Rowley <david.rowley@2ndquadrant.com>)
Responses Re: pg_dump multi VALUES INSERT  (David Rowley <david.rowley@2ndquadrant.com>)
Re: pg_dump multi VALUES INSERT  (Surafel Temesgen <surafel3000@gmail.com>)
Re: pg_dump multi VALUES INSERT  (Surafel Temesgen <surafel3000@gmail.com>)
List 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.


pgsql-hackers by date:

Previous
From: Noah Misch
Date:
Subject: Re: pgsql: Remove references to Majordomo
Next
From: Noah Misch
Date:
Subject: Spurious "apparent wraparound" via SimpleLruTruncate() rounding