Обсуждение: pg_dump and inserts

I just checked and pg_dump -d _doesn't_ place the INSERT's in a
transsaction.  Seems it should, and perhaps add a:
SET autocommit TO 'on'

as well.  Of course, that SET would fail when restoring to prior
releases, but they don't have autocommit off anyway so it can be
ignored.  Comments?  This would certainly speed up loads that use
INSERT.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073
 

Bruce Momjian wrote:
> I just checked and pg_dump -d _doesn't_ place the INSERT's in a
> transsaction.  Seems it should, and perhaps add a:
> 
>     SET autocommit TO 'on'
> 
> as well.  Of course, that SET would fail when restoring to prior
> releases, but they don't have autocommit off anyway so it can be
> ignored.  Comments?  This would certainly speed up loads that use
> INSERT.

I'm not sure that pg_dump is the right place to do this, unless it's
something that can be turned on/off with a command line switch
(remember that editing the file to delete or comment out the
transaction commands isn't necessarily feasible).  It seems to me that
a DBA might want to have a bit more control over this behavior.

So: if pg_restore or some other utility is used to perform the
restore, then that utility should issue the BEGIN/END on behalf of the
user.

One reason I can think of for keeping manual control over the
transaction is the case where one wishes to restore from multiple
dumps.  In that case, it could be very useful to issue a single
transaction block around the entire thing, and to examine the restored
data before actually committing the results, in case something doesn't
look right.


This is all complicated, of course, by commands which cannot occur
within transactions, which is why I think a switch controlling this
behavior is appropriate.  I certainly don't have a problem with the
default being that the transaction commands are issued in the dump, as
long as it's a behavior that can be turned off.


-- 
Kevin Brown                          kevin@sysexperts.com

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I just checked and pg_dump -d _doesn't_ place the INSERT's in a
> transsaction.  Seems it should,

I think this is a bad idea.  If one were after speed, one would have
used the COPY format in the first place.  If one uses INSERTs, there
may be a reason for it --- like, say, wanting each row insertion to
succeed or fail independently.  Put a begin/end around it, and you
lose that.

> and perhaps add a:
>     SET autocommit TO 'on'
> as well.

This is probably a good idea, since pg_dump scripts effectively assume
that anyway.

> Of course, that SET would fail when restoring to prior
> releases,

Irrelevant; current pg_dump scripts already issue a SET that pre-7.3
servers won't recognize (search_path).  A failed SET is harmless anyway,
or should be.  (What we really need is for someone to fix pg_restore to
not abort on SQL errors...)
        regards, tom lane

Attached is a patch the more clearly handles autocommit in pg_dump.  I
had already fixed pg_dump for doing autocommit while dumping, but didn't
handle setting autocommit on restore.

I focused on the initial script file startup, every \\connect,
pg_restore, and pg_dumpall.  I think I got them all.

New pg_dump output is:

    --
    -- PostgreSQL database dump
    --

    SET autocommit TO 'on';

    \connect - postgres

    SET autocommit TO 'on';

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I just checked and pg_dump -d _doesn't_ place the INSERT's in a
> > transsaction.  Seems it should,
>
> I think this is a bad idea.  If one were after speed, one would have
> used the COPY format in the first place.  If one uses INSERTs, there
> may be a reason for it --- like, say, wanting each row insertion to
> succeed or fail independently.  Put a begin/end around it, and you
> lose that.
>
> > and perhaps add a:
> >     SET autocommit TO 'on'
> > as well.
>
> This is probably a good idea, since pg_dump scripts effectively assume
> that anyway.
>
> > Of course, that SET would fail when restoring to prior
> > releases,
>
> Irrelevant; current pg_dump scripts already issue a SET that pre-7.3
> servers won't recognize (search_path).  A failed SET is harmless anyway,
> or should be.  (What we really need is for someone to fix pg_restore to
> not abort on SQL errors...)
>
>             regards, tom lane
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/bin/pg_dump/pg_backup_archiver.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_backup_archiver.c,v
retrieving revision 1.67
diff -c -c -r1.67 pg_backup_archiver.c
*** src/bin/pg_dump/pg_backup_archiver.c    1 Feb 2003 22:06:59 -0000    1.67
--- src/bin/pg_dump/pg_backup_archiver.c    14 Feb 2003 19:39:08 -0000
***************
*** 206,212 ****
          sav = SetOutput(AH, ropt->filename, ropt->compression);

      ahprintf(AH, "--\n-- PostgreSQL database dump\n--\n\n");
!
      /*
       * Drop the items at the start, in reverse order
       */
--- 206,213 ----
          sav = SetOutput(AH, ropt->filename, ropt->compression);

      ahprintf(AH, "--\n-- PostgreSQL database dump\n--\n\n");
!     ahprintf(AH, "SET autocommit TO 'on';\n\n");
!
      /*
       * Drop the items at the start, in reverse order
       */
***************
*** 2109,2115 ****
                            dbname ? fmtId(dbname) : "-");
          appendPQExpBuffer(qry, " %s\n\n",
                            fmtId(user));
!
          ahprintf(AH, qry->data);

          destroyPQExpBuffer(qry);
--- 2110,2117 ----
                            dbname ? fmtId(dbname) : "-");
          appendPQExpBuffer(qry, " %s\n\n",
                            fmtId(user));
!         appendPQExpBuffer(qry, "SET autocommit TO 'on';\n\n");
!
          ahprintf(AH, qry->data);

          destroyPQExpBuffer(qry);
Index: src/bin/pg_dump/pg_backup_db.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_backup_db.c,v
retrieving revision 1.45
diff -c -c -r1.45 pg_backup_db.c
*** src/bin/pg_dump/pg_backup_db.c    13 Feb 2003 04:54:15 -0000    1.45
--- src/bin/pg_dump/pg_backup_db.c    14 Feb 2003 19:39:09 -0000
***************
*** 213,218 ****
--- 213,233 ----
      if (password)
          free(password);

+     /* check for version mismatch */
+     _check_database_version(AH, true);
+
+     /* Turn autocommit on */
+     if (AH->public.remoteVersion >= 70300)
+     {
+         PGresult   *res;
+
+         res = PQexec(AH->connection, "SET autocommit TO 'on'");
+         if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
+             die_horribly(AH, NULL, "SET autocommit TO 'on' failed: %s",
+                           PQerrorMessage(AH->connection));
+         PQclear(res);
+     }
+
      PQsetNoticeProcessor(newConn, notice_processor, NULL);

      return newConn;
Index: src/bin/pg_dump/pg_dumpall.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_dumpall.c,v
retrieving revision 1.13
diff -c -c -r1.13 pg_dumpall.c
*** src/bin/pg_dump/pg_dumpall.c    16 Jan 2003 15:27:59 -0000    1.13
--- src/bin/pg_dump/pg_dumpall.c    14 Feb 2003 19:39:09 -0000
***************
*** 190,195 ****
--- 190,196 ----
      printf("-- PostgreSQL database cluster dump\n");
      printf("--\n\n");
      printf("\\connect \"template1\"\n\n");
+     printf("SET autocommit TO 'on';\n\n");

      dumpUsers(conn);
      dumpGroups(conn);
***************
*** 552,557 ****
--- 553,559 ----
              fprintf(stderr, _("%s: dumping database \"%s\"...\n"), progname, dbname);

          printf("\\connect %s\n", fmtId(dbname));
+         printf("SET autocommit TO 'on';\n\n");
          ret = runPgDump(dbname);
          if (ret != 0)
          {
***************
*** 676,681 ****
--- 678,691 ----
          }
      }
      PQclear(res);
+
+     if (server_version >= 70300)
+     {
+         PGresult   *res;
+
+         res = executeQuery(conn, "SET autocommit TO 'on';SELECT 1;");
+         PQclear(res);
+     }

      return conn;
  }