Message-ID: <81f71541075c376ce137ee6aec42c61fb9a60a3d.camel@cybertec.at>
Subject: Re: Improve documentation for pg_upgrade, standbys and rsync
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Stephen Frost <sfrost@snowman.net>
Cc: pgsql-docs@lists.postgresql.org
Date: Wed, 18 Aug 2021 14:24:13 +0200
In-Reply-To: <20210726191126.GW20766@tamriel.snowman.net>
References: <22f129004bb66cd91e1dfd3345a9787f5039f3ae.camel@cybertec.at>
	 <20210519143135.GI20766@tamriel.snowman.net>
	 <c176f23cdde4d4ff0010b5e7a517c1b371500279.camel@cybertec.at>
	 <20210716131744.GA20766@tamriel.snowman.net>
	 <a1a9cf4ebc8c674f4165af376c522f52ab02a3f6.camel@cybertec.at>
	 <20210726191126.GW20766@tamriel.snowman.net>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/81f71541075c376ce137ee6aec42c61fb9a60a3d.camel%40cybertec.at>
Precedence: bulk

On Mon, 2021-07-26 at 15:11 -0400, Stephen Frost wrote:
> > > > > An additional thing that we should really be mentioning is to tell
> > > > > people to go in and TRUNCATE all of their UNLOGGED tables before going
> > > > > through this process, otherwise the rsync will end up spending a bunch
> > > > > of time copying the files for UNLOGGED relations which you really don't
> > > > > want.
> > 
> > Ok, done.
> 
> Great, thanks, it's not quite this simple, unfortunately, more below..
>
> > +    <para>
> > +     If you are upgrading standby servers using methods outlined in section <xref
> > +     linkend="pgupgrade-step-replicas"/>, you should consider dropping temporary
> > +     tables and truncating unlogged tables on the primary, since that will speed up
> > +     <application>rsync</application> and keep the down time short.
> > +     You could run the following <application>psql</application> commands
> > +     in all databases:
> > +
> > +<programlisting>
> > +SELECT format('DROP TABLE %s', oid::regclass) FROM pg_class WHERE relpersistence = 't' \gexec
> > +SELECT format('TRUNCATE %s', oid::regclass) FROM pg_class WHERE relpersistence = 'u' \gexec
> > +</programlisting>
> 
> Temporary tables aren't actually visible across different backends, nor
> should they exist once the system has been shut down, but sometimes they
> do get left around due to a crash, so the above won't actually work and
> isn't the way to deal with those.  The same can also happen with
> temporary files that we create which end up in pgsql_tmp.
> 
> We could possibly exclude pgsql_tmp in the rsync command, but cleaning
> up the temporary table files would involve something more complicated
> like using 'find' to search for any '^t[0-9]+_[0-9]+.*$' files or
> something along those lines.
> 
> Though, for that matter we should really be looking through all of the
> directories and files that pg_basebackup excludes and considering if
> they should somehow be excluded.  There's no easy way to exclude
> everything that pg_basebackup would with just an rsync because the logic
> is a bit complicated (which is why I was saying we really need a proper
> tool...) but we could probably provide a somewhat better rsync command
> by going through that list and excluding what makes sense to exclude.
> We could also provide another explicit before-rsync step to review all
> the temp table files and move them or remove them, depending on how
> comfortable one is with hacking around in the data directory.
> 
> This, of course, all comes back to the original complaint I had about
> documenting this approach, which is that these things should only be
> done by someone extremely familiar with the PG codebase, until and
> unless we write an actual tool to do this.

I agree with what you write, but that sounds like you are arguing for
a code patch rather than for documentation to enable the user to do
that manually, which is what I believe you said initially.

My two statements will get rid of temporary tables left behind after
a crash and truncate unlogged tables, which should be an improvement.

Of course it would be good to get rid of orphaned files left behind
after a crash, but, as you say, that is not so easy.

I'd say that writing tools to do better than my two SQL statements
is nice to have, but beyond the scope of this documentation patch.

> > > > Recommend using the --relative option of rsync for clarity
> > > > and adapt the code samples accordingly.
> > > > Using relative paths makes clearer what is meant by "current
> > > > directory" and "remote_dir".
> > 
> > I normally prefer absolute paths as well.
> > But that is the only way I got it to run, and I think that in this
> > case it adds clarity to have the data directories relative to your
> > current working directory.
> 
> I'm pretty curious that you weren't able to get it to run with absolute
> paths..

I tried a couple of times with a test cluster and failed.

Part of the confustion for me is that you are supposed to run the
rsync from a certain directory, which seems weird if paths are absolute.
Run from *any* directory above the old and the new cluster?

"Relative to my current directory" makes more sense to me here.

> > +     (There will be a mismatch if old standby servers were shut down
> > +     before the old primary or if the old standby servers are still running.)
> 
> Would probably be good to note that if the standby's were shut down
> before the primary then this method can *not* be used safely...  The
> above leaves it unclear about if the mismatch is an issue or not.  I get
> that this was in the original docs, but still would be good to improve
> it.

Agreed.

Yours,
Laurenz Albe