On Tue, Dec 25, 2018 at 2:47 PM Fabien COELHO <coe...@cri.ensmp.fr> wrote:
Thank you for looking into it > > Hello Surafel, > > > Thank you for informing, Here is an updated patch against current master > > Patch applies cleanly, compiles, "make check" is okay, but given that the > feature is not tested... > > Feature should be tested somewhere. > > ISTM that command-line switches with optional arguments should be avoided: > This feature is seldom used (hmmm... 2 existing instances), because it > interferes with argument processing if such switches are used as the last > one. It is only okay with commands which do not expect arguments. For > backward compatibility, this suggests to add another switch, eg > --insert-multi=100 or whatever, which would possibly default to 100. The > alternative is to break compatibility with adding a mandatory argument, > but I guess it would not be admissible to committers. > > Function "atoi" parses "1zzz" as 1, which is debatable, so I'd suggest to > avoid it and use some stricter option and error out on malformed integers. > > The --help output does not document the --inserts argument, nor the > documentation. > > done There is an indendation issue within the while loop. > > Given that the implementation is largely a copy-paste of the preceding > function, I'd suggest to simply extend it so that it takes into account > the "multi insert" setting and default to the previous behavior if not > set. At first i also try to do it like that but it seems the function will became long and more complex to me Regards Surafel
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index 2015410a42..ee94d1d293 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -775,6 +775,18 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>--insert-multi</option></term> + <listitem> + <para> + Specify the number of values per <command>INSERT</command> command. + This will make the dump file smaller than <option>--inserts</option> + and it is faster to reload but lack per row data lost on error + instead entire affected insert statement data lost. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>--load-via-partition-root</option></term> <listitem> diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h index 4a2e122e2d..73a243ecb0 100644 --- a/src/bin/pg_dump/pg_backup.h +++ b/src/bin/pg_dump/pg_backup.h @@ -72,6 +72,7 @@ typedef struct _restoreOptions int dropSchema; int disable_dollar_quoting; int dump_inserts; + int dump_inserts_multiple; int column_inserts; int if_exists; int no_comments; /* Skip comments */ @@ -144,6 +145,7 @@ typedef struct _dumpOptions /* flags for various command-line long options */ int disable_dollar_quoting; int dump_inserts; + int dump_inserts_multiple; int column_inserts; int if_exists; int no_comments; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 637c79af48..d433c3882a 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -313,6 +313,7 @@ main(int argc, char **argv) int plainText = 0; ArchiveFormat archiveFormat = archUnknown; ArchiveMode archiveMode; + char *p; static DumpOptions dopt; @@ -359,6 +360,7 @@ main(int argc, char **argv) {"exclude-table-data", required_argument, NULL, 4}, {"if-exists", no_argument, &dopt.if_exists, 1}, {"inserts", no_argument, &dopt.dump_inserts, 1}, + {"insert-multi", required_argument, NULL, 8}, {"lock-wait-timeout", required_argument, NULL, 2}, {"no-tablespaces", no_argument, &dopt.outputNoTablespaces, 1}, {"quote-all-identifiers", no_argument, "e_all_identifiers, 1}, @@ -557,6 +559,27 @@ main(int argc, char **argv) dosync = false; break; + case 8: /* inserts values number */ + errno = 0; + dopt.dump_inserts_multiple = strtol(optarg, &p, 10); + if (p == optarg || *p != '\0') + { + write_msg(NULL, "argument of --insert-multi must be a number\n"); + exit_nicely(1); + } + if (errno == ERANGE) + { + write_msg(NULL, "argument of --insert-multi exceeds integer range.\n"); + exit_nicely(1); + } + if (dopt.dump_inserts_multiple < 0) + { + write_msg(NULL, "argument of --insert-multi must be positive number\n"); + exit_nicely(1); + } + + break; + default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); exit_nicely(1); @@ -607,8 +630,9 @@ main(int argc, char **argv) if (dopt.if_exists && !dopt.outputClean) exit_horribly(NULL, "option --if-exists requires option -c/--clean\n"); - if (dopt.do_nothing && !(dopt.dump_inserts || dopt.column_inserts)) - exit_horribly(NULL, "option --on-conflict-do-nothing requires option --inserts or --column-inserts\n"); + if (dopt.do_nothing && !(dopt.dump_inserts || dopt.column_inserts || + dopt.dump_inserts_multiple)) + exit_horribly(NULL, "option --on-conflict-do-nothing requires option --inserts , --insert-multi or --column-inserts\n"); /* Identify archive format to emit */ archiveFormat = parseArchiveFormat(format, &archiveMode); @@ -877,6 +901,7 @@ main(int argc, char **argv) ropt->use_setsessauth = dopt.use_setsessauth; ropt->disable_dollar_quoting = dopt.disable_dollar_quoting; ropt->dump_inserts = dopt.dump_inserts; + ropt->dump_inserts_multiple = dopt.dump_inserts_multiple; ropt->no_comments = dopt.no_comments; ropt->no_publications = dopt.no_publications; ropt->no_security_labels = dopt.no_security_labels; @@ -967,6 +992,7 @@ help(const char *progname) printf(_(" --exclude-table-data=TABLE do NOT dump data for the named table(s)\n")); printf(_(" --if-exists use IF EXISTS when dropping objects\n")); printf(_(" --inserts dump data as INSERT commands, rather than COPY\n")); + printf(_(" --insert-multi number of values per INSERT command\n")); printf(_(" --load-via-partition-root load partitions via the root table\n")); printf(_(" --no-comments do not dump comments\n")); printf(_(" --no-publications do not dump publications\n")); @@ -2052,6 +2078,193 @@ dumpTableData_insert(Archive *fout, void *dcontext) return 1; } +/* + * Dump table data using multiple values INSERT commands. + */ +static int +dumpTableData_insert_multiple(Archive *fout, void *dcontext) +{ + TableDataInfo *tdinfo = (TableDataInfo *) dcontext; + TableInfo *tbinfo = tdinfo->tdtable; + DumpOptions *dopt = fout->dopt; + PQExpBuffer q = createPQExpBuffer(); + PQExpBuffer i = createPQExpBuffer(); + PQExpBuffer insertStmt = NULL; + PGresult *res; + int tuple; + int nfields; + int field; + int ntuple; + int ltuple; + + appendPQExpBuffer(q, "DECLARE _pg_dump_cursor CURSOR FOR " + "SELECT * FROM ONLY %s", + fmtQualifiedDumpable(tbinfo)); + if (tdinfo->filtercond) + appendPQExpBuffer(q, " %s", tdinfo->filtercond); + + ExecuteSqlStatement(fout, q->data); + appendPQExpBuffer(i, "FETCH %d FROM _pg_dump_cursor", + dopt->dump_inserts_multiple); + while (1) + { + res = ExecuteSqlQuery(fout, i->data, PGRES_TUPLES_OK); + nfields = PQnfields(res); + ntuple = PQntuples(res); + ltuple = ntuple-1; + if (ntuple > 0) + { + if (insertStmt == NULL) + { + TableInfo *targettab; + + insertStmt = createPQExpBuffer(); + + /* + * When load-via-partition-root is set, get the root table + * name for the partition table, so that we can reload data + * through the root table. + */ + if (dopt->load_via_partition_root && tbinfo->ispartition) + targettab = getRootTableInfo(tbinfo); + else + targettab = tbinfo; + + appendPQExpBuffer(insertStmt, "INSERT INTO %s ", + fmtQualifiedDumpable(targettab)); + + /* corner case for zero-column table */ + if (nfields == 0) + { + appendPQExpBufferStr(insertStmt, "DEFAULT VALUES;\n"); + } + else + { + /* append the list of column names if required */ + if (dopt->column_inserts) + { + appendPQExpBufferChar(insertStmt, '('); + for (field = 0; field < nfields; field++) + { + if (field > 0) + appendPQExpBufferStr(insertStmt, ", "); + appendPQExpBufferStr(insertStmt, + fmtId(PQfname(res, field))); + } + appendPQExpBufferStr(insertStmt, ") "); + } + + if (tbinfo->needs_override) + appendPQExpBufferStr(insertStmt, "OVERRIDING SYSTEM VALUE "); + + appendPQExpBufferStr(insertStmt, "VALUES "); + } + } + archputs(insertStmt->data, fout); + for (tuple = 0; tuple < ntuple ; tuple++) + { + + /* if it is zero-column table then we're done */ + if (nfields == 0) + continue; + if (tuple == 0) + archputs("(", fout); + else + archputs(", (", fout); + + for (field = 0; field < nfields; field++) + { + if (field > 0) + archputs(", ", fout); + if (PQgetisnull(res, tuple, field)) + { + archputs("NULL", fout); + continue; + } + + /* XXX This code is partially duplicated in ruleutils.c */ + switch (PQftype(res, field)) + { + case INT2OID: + case INT4OID: + case INT8OID: + case OIDOID: + case FLOAT4OID: + case FLOAT8OID: + case NUMERICOID: + { + /* + * These types are printed without quotes unless + * they contain values that aren't accepted by the + * scanner unquoted (e.g., 'NaN'). Note that + * strtod() and friends might accept NaN, so we + * can't use that to test. + * + * In reality we only need to defend against + * infinity and NaN, so we need not get too crazy + * about pattern matching here. + */ + const char *s = PQgetvalue(res, tuple, field); + + if (strspn(s, "0123456789 +-eE.") == strlen(s)) + archputs(s, fout); + else + archprintf(fout, "'%s'", s); + } + break; + + case BITOID: + case VARBITOID: + archprintf(fout, "B'%s'", + PQgetvalue(res, tuple, field)); + break; + + case BOOLOID: + if (strcmp(PQgetvalue(res, tuple, field), "t") == 0) + archputs("true", fout); + else + archputs("false", fout); + break; + + default: + /* All other types are printed as string literals. */ + resetPQExpBuffer(q); + appendStringLiteralAH(q, + PQgetvalue(res, tuple, field), + fout); + archputs(q->data, fout); + break; + } + } + if (tuple < ltuple) + archputs(")\n", fout); + + } + if (!dopt->do_nothing) + archputs(");\n", fout); + else + archputs(") ON CONFLICT DO NOTHING;\n", fout); + } + if (PQntuples(res) <= 0) + { + PQclear(res); + break; + } + PQclear(res); + } + + archputs("\n\n", fout); + + ExecuteSqlStatement(fout, "CLOSE _pg_dump_cursor"); + + destroyPQExpBuffer(q); + destroyPQExpBuffer(i); + if (insertStmt != NULL) + destroyPQExpBuffer(insertStmt); + + return 1; +} + /* * getRootTableInfo: * get the root TableInfo for the given partition table. @@ -2091,7 +2304,7 @@ dumpTableData(Archive *fout, TableDataInfo *tdinfo) char *copyStmt; const char *copyFrom; - if (!dopt->dump_inserts) + if (!dopt->dump_inserts && !dopt->dump_inserts_multiple) { /* Dump/restore using COPY */ dumpFn = dumpTableData_copy; @@ -2118,6 +2331,12 @@ dumpTableData(Archive *fout, TableDataInfo *tdinfo) fmtCopyColumnList(tbinfo, clistBuf)); copyStmt = copyBuf->data; } + else if (dopt->dump_inserts_multiple) + { + /* Restore using multiple values INSERT */ + dumpFn = dumpTableData_insert_multiple; + copyStmt = NULL; + } else { /* Restore using INSERT */ diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl index a875d540b8..e1ca5416b7 100644 --- a/src/bin/pg_dump/t/001_basic.pl +++ b/src/bin/pg_dump/t/001_basic.pl @@ -118,8 +118,8 @@ command_fails_like( command_fails_like( [ 'pg_dump', '--on-conflict-do-nothing' ], - qr/\Qpg_dump: option --on-conflict-do-nothing requires option --inserts or --column-inserts\E/, - 'pg_dump: option --on-conflict-do-nothing requires option --inserts or --column-inserts'); + qr/\Qpg_dump: option --on-conflict-do-nothing requires option --inserts , --insert-multi or --column-inserts\E/, + 'pg_dump: option --on-conflict-do-nothing requires option --inserts , --insert-multi or --column-inserts'); # pg_dumpall command-line argument checks command_fails_like(