Re: Use of fsync; was Re: Pg_upgrade speed for many tables

Applied.

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

On Fri, Nov 30, 2012 at 10:43:29PM -0500, Bruce Momjian wrote:
> On Mon, Nov 26, 2012 at 02:43:19PM -0500, Bruce Momjian wrote:
> > > >> In any event, I think the documentation should caution that the
> > > >> upgrade should not be deemed to be a success until after a system-wide
> > > >> sync has been done.  Even if we use the link rather than copy method,
> > > >> are we sure that that is safe if the directories recording those links
> > > >> have not been fsynced?
> > > >
> > > > OK, the above is something I have been thinking about, and obviously you
> > > > have too.  If you change fsync from off to on in a cluster, and restart
> > > > it, there is no guarantee that the dirty pages you read from the kernel
> > > > are actually on disk, because Postgres doesn't know they are dirty.
> > > > They probably will be pushed to disk by the kernel in less than one
> > > > minute, but still, it doesn't seem reliable. Should this be documented
> > > > in the fsync section?
> > > >
> > > > Again, another reason not to use fsync=off, though your example of the
> > > > file copy is a good one.  As you stated, this is a problem with the file
> > > > copy/link, independent of how Postgres handles the files.  We can tell
> > > > people to use 'sync' as root on Unix, but what about Windows?
> > > 
> > > I'm pretty sure someone mentioned the way to do that on Windows in
> > > this list in the last few months, but I can't seem to find it.  I
> > > thought it was the initdb fsync thread.
> > 
> > Yep, the code is already in initdb to fsync a directory --- we just need
> > a way for pg_upgrade to access it.
> 
> I have developed the attached patch that does this.  It basically adds
> an --sync-only option to initdb, then turns off all durability in
> pg_upgrade and has pg_upgrade run initdb --sync-only;  this give us
> another nice speedup!
> 
>              ------ SSD ---- -- magnetic ---
>                 git    patch    git    patch
>         1      11.11   11.11   11.10   11.13
>      1000      20.57   19.89   20.72   19.30
>      2000      28.02   25.81   28.50   27.53
>      4000      42.00   43.59   46.71   46.84
>      8000      89.66   74.16   89.10   73.67
>     16000     157.66  135.98  159.97  153.48
>     32000     316.24  296.90  334.74  308.59
>     64000     814.97  715.53  797.34  727.94
> 
> (I am very happy with these times.  Thanks to Jeff Janes for his
> suggestions.)
> 
> I have also added documentation to the 'fsync' configuration variable
> warning about dirty buffers and recommending flushing them to disk
> before the cluster is crash-recovery safe.
> 
> I consider this patch ready for 9.3 application (meaning it is not a
> prototype).
> 
> -- 
>   Bruce Momjian  <bruce@momjian.us>        http://momjian.us
>   EnterpriseDB                             http://enterprisedb.com
> 
>   + It's impossible for everything to be true. +

> diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
> new file mode 100644
> index c12f15b..63df529
> *** a/contrib/pg_upgrade/pg_upgrade.c
> --- b/contrib/pg_upgrade/pg_upgrade.c
> *************** main(int argc, char **argv)
> *** 150,155 ****
> --- 150,161 ----
>                 new_cluster.pgdata);
>       check_ok();
>   
> +     prep_status("Sync data directory to disk");
> +     exec_prog(UTILITY_LOG_FILE, NULL, true,
> +               "\"%s/initdb\" --sync-only \"%s\"", new_cluster.bindir,
> +               new_cluster.pgdata);
> +     check_ok();
> + 
>       create_script_for_cluster_analyze(&analyze_script_file_name);
>       create_script_for_old_cluster_deletion(&deletion_script_file_name);
>   
> diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
> new file mode 100644
> index 49d4c8f..05d8cc0
> *** a/contrib/pg_upgrade/server.c
> --- b/contrib/pg_upgrade/server.c
> *************** start_postmaster(ClusterInfo *cluster)
> *** 209,217 ****
>        * a gap of 2000000000 from the current xid counter, so autovacuum will
>        * not touch them.
>        *
> !      *    synchronous_commit=off improves object creation speed, and we only
> !      *    modify the new cluster, so only use it there.  If there is a crash,
> !      *    the new cluster has to be recreated anyway.
>        */
>       snprintf(cmd, sizeof(cmd),
>                "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s%s%s\" start",
> --- 209,217 ----
>        * a gap of 2000000000 from the current xid counter, so autovacuum will
>        * not touch them.
>        *
> !      * Turn off durability requirements to improve object creation speed, and
> !      * we only modify the new cluster, so only use it there.  If there is a
> !      * crash, the new cluster has to be recreated anyway.
>        */
>       snprintf(cmd, sizeof(cmd),
>                "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s%s%s\" start",
> *************** start_postmaster(ClusterInfo *cluster)
> *** 219,225 ****
>                (cluster->controldata.cat_ver >=
>                 BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
>                " -c autovacuum=off -c autovacuum_freeze_max_age=2000000000",
> !              (cluster == &new_cluster) ? " -c synchronous_commit=off" : "",
>                cluster->pgopts ? cluster->pgopts : "", socket_string);
>   
>       /*
> --- 219,226 ----
>                (cluster->controldata.cat_ver >=
>                 BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
>                " -c autovacuum=off -c autovacuum_freeze_max_age=2000000000",
> !              (cluster == &new_cluster) ?
> !                 " -c synchronous_commit=off -c fsync=off -c full_page_writes=off" : "",
>                cluster->pgopts ? cluster->pgopts : "", socket_string);
>   
>       /*
> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> new file mode 100644
> index b56070b..b7df8ce
> *** a/doc/src/sgml/config.sgml
> --- b/doc/src/sgml/config.sgml
> *************** include 'filename'
> *** 1697,1702 ****
> --- 1697,1711 ----
>          </para>
>   
>          <para>
> +         For reliable recovery when changing <varname>fsync</varname>
> +         off to on, it is necessary to force all modified buffers in the
> +         kernel to durable storage.  This can be done while the cluster
> +         is shutdown or while fsync is on by running <command>initdb
> +         --sync-only</command>, running <command>sync</>, unmounting the
> +         file system, or rebooting the server.
> +        </para>
> + 
> +        <para>
>           In many situations, turning off <xref linkend="guc-synchronous-commit">
>           for noncritical transactions can provide much of the potential
>           performance benefit of turning off <varname>fsync</varname>, without
> diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
> new file mode 100644
> index 08ee37e..a1e46eb
> *** a/doc/src/sgml/ref/initdb.sgml
> --- b/doc/src/sgml/ref/initdb.sgml
> *************** PostgreSQL documentation
> *** 245,250 ****
> --- 245,261 ----
>        </varlistentry>
>   
>        <varlistentry>
> +       <term><option>-S</option></term>
> +       <term><option>--sync-only</option></term>
> +       <listitem>
> +        <para>
> +         Safely write all database files to disk and exit.  This does not
> +         perform any of the normal <application>initdb</> operations.
> +        </para>
> +       </listitem>
> +      </varlistentry>
> + 
> +      <varlistentry>
>         <term><option>-T <replaceable>CFG</></option></term>
>         <term><option>--text-search-config=<replaceable>CFG</></option></term>
>         <listitem>
> diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
> new file mode 100644
> index 402504b..8c0a9f4
> *** a/src/bin/initdb/initdb.c
> --- b/src/bin/initdb/initdb.c
> *************** static const char *authmethodlocal = "";
> *** 118,123 ****
> --- 118,124 ----
>   static bool debug = false;
>   static bool noclean = false;
>   static bool do_sync = true;
> + static bool sync_only = false;
>   static bool show_setting = false;
>   static char *xlog_dir = "";
>   
> *************** usage(const char *progname)
> *** 2796,2801 ****
> --- 2797,2803 ----
>       printf(_("  -n, --noclean             do not clean up after errors\n"));
>       printf(_("  -N, --nosync              do not wait for changes to be written safely to disk\n"));
>       printf(_("  -s, --show                show internal settings\n"));
> +     printf(_("  -S, --sync-only           only sync data directory\n"));
>       printf(_("\nOther options:\n"));
>       printf(_("  -V, --version             output version information, then exit\n"));
>       printf(_("  -?, --help                show this help, then exit\n"));
> *************** main(int argc, char *argv[])
> *** 3445,3450 ****
> --- 3447,3453 ----
>           {"show", no_argument, NULL, 's'},
>           {"noclean", no_argument, NULL, 'n'},
>           {"nosync", no_argument, NULL, 'N'},
> +         {"sync-only", no_argument, NULL, 'S'},
>           {"xlogdir", required_argument, NULL, 'X'},
>           {NULL, 0, NULL, 0}
>       };
> *************** main(int argc, char *argv[])
> *** 3476,3482 ****
>   
>       /* process command-line options */
>   
> !     while ((c = getopt_long(argc, argv, "dD:E:L:nNU:WA:sT:X:", long_options, &option_index)) != -1)
>       {
>           switch (c)
>           {
> --- 3479,3485 ----
>   
>       /* process command-line options */
>   
> !     while ((c = getopt_long(argc, argv, "dD:E:L:nNU:WA:sST:X:", long_options, &option_index)) != -1)
>       {
>           switch (c)
>           {
> *************** main(int argc, char *argv[])
> *** 3522,3527 ****
> --- 3525,3533 ----
>               case 'N':
>                   do_sync = false;
>                   break;
> +             case 'S':
> +                 sync_only = true;
> +                 break;
>               case 'L':
>                   share_path = pg_strdup(optarg);
>                   break;
> *************** main(int argc, char *argv[])
> *** 3589,3594 ****
> --- 3595,3608 ----
>           exit(1);
>       }
>   
> +     /* If we only need to fsync, just to it and exit */
> +     if (sync_only)
> +     {
> +         setup_pgdata();
> +         perform_fsync();
> +         return 0;
> +     }
> +     
>       if (pwprompt && pwfilename)
>       {
>           fprintf(stderr, _("%s: password prompt and password file cannot be specified together\n"), progname);

> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +