agora inbox for pgsql-hackers@postgresql.org  
help / color / mirror / Atom feed
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Steve Singer <steve@ssinger.info>
Cc: pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: Wed, 27 Jan 2021 15:35:03 -0600
Message-ID: <20210127213503.GX30745@telsasoft.com> (raw)
In-Reply-To: <ef752a011a4431a9a6ae6f8c1e2eb001@postgrespro.ru>
References: <bac6739d47f053fe04fbd8cf05283137@postgrespro.ru>
	<e8f209aebf33f02ea118e509a935647a@postgrespro.ru>
	<20210121212651.GY8560@telsasoft.com>
	<944df7cc452fe0c34cf7820da4588d05@postgrespro.ru>
	<YA58Qdgb3a7/yrOH@paquier.xyz>
	<690fa051803c071d213b0d07b5aa9f55@postgrespro.ru>
	<YA+9mAMWYLXJMVPL@paquier.xyz>
	<c36e51a07183b95ddd287be4ee4cfbbd@postgrespro.ru>
	<YBDamQI7NCr7JBJA@paquier.xyz>
	<ef752a011a4431a9a6ae6f8c1e2eb001@postgrespro.ru>

Thanks for updating the patch.  I have just a couple comments on the new (and
old) language.

On Thu, Jan 28, 2021 at 12:19:06AM +0300, Alexey Kondratov wrote:
> Also added tests for ACL checks, relfilenode changes. Added ACL recheck for
> multi-transactional case. Added info about TOAST index reindexing. Changed
> some comments.

> +      Specifies that indexes will be rebuilt on a new tablespace.
> +      Cannot be used with "mapped" and system (unless <varname>allow_system_table_mods</varname>

say mapped *or* system relations
Or maybe:
mapped or (unless >allow_system_table_mods<) system relations.

> +      is set to <literal>TRUE</literal>) relations. If <literal>SCHEMA</literal>,
> +      <literal>DATABASE</literal> or <literal>SYSTEM</literal> are specified,
> +      then all "mapped" and system relations will be skipped and a single
> +      <literal>WARNING</literal> will be generated. Indexes on TOAST tables
> +      are reindexed, but not moved the new tablespace.

moved *to* the new tablespace.
I don't know if that needs to be said at all.  We talked about it a lot to
arrive at the current behavior, but I think that's only due to the difficulty
of correcting the initial mistake.

> +	/*
> +	 * Set the new tablespace for the relation.  Do that only in the
> +	 * case where the reindex caller wishes to enforce a new tablespace.

I'd say just "/* Set new tablespace, if requested */
You wrote something similar in an earlier revision of your refactoring patch.

> +		 * Mark the relation as ready to be dropped at transaction commit,
> +		 * before making visible the new tablespace change so as this won't
> +		 * miss things.

This comment is vague.  I think Michael first wrote this comment about a year
ago.  Does it mean "so the new tablespace won't be missed" ?  Missed by what ?

-- 
Justin





view thread (154+ messages)  latest in thread

Message-ID: <20210127213503.GX30745@telsasoft.com>
Permalink:  ../20210127213503.GX30745@telsasoft.com/
Also on:    postgresql.org/message-id/20210127213503.GX30745@telsasoft.com

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: pryzby@telsasoft.com, a.kondratov@postgrespro.ru, michael@paquier.xyz, alvherre@alvh.no-ip.org, peter.eisentraut@enterprisedb.com, masahiko.sawada@2ndquadrant.com, steve@ssinger.info, robertmhaas@gmail.com, a.korotkov@postgrespro.ru, sawada.mshk@gmail.com, jltallon@adv-solutions.net
  Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
  In-Reply-To: <20210127213503.GX30745@telsasoft.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox