Date: Mon, 18 Jul 2022 21:26:53 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Simon Riggs <simon.riggs@enterprisedb.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Bernd Helmle <mailings@oopsware.de>,
	Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
	pgsql-hackers@postgresql.org,
	Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Subject: Re: Allowing REINDEX to have an optional name
Message-ID: <20220719022652.GE12702@telsasoft.com>
References: <Ypf+GU4zhvRTirJ1@paquier.xyz>
 <CANbhV-E_qSxF4JHdswaKcaG_Mo25h5WyXBPYWyZ0+=Mc-QxczQ@mail.gmail.com>
 <CANbhV-EmOh-XTssH5TwannC2S1VDN5zK-b+oZKs8UdyUtc2oUw@mail.gmail.com>
 <YrvWoAsaX/gUG2re@paquier.xyz>
 <CANbhV-EL4+RK49Qmk66zFmTiEEhT6TKia0ow_t5zogV1-FfAEw@mail.gmail.com>
 <1386216.1656884491@sss.pgh.pa.us>
 <YsKP6wJ0+XvF+6/8@paquier.xyz>
 <CANbhV-H2Y=jM1QVRkT-OOa2+KPTd3po+=2LmZ35DmTidjRovGg@mail.gmail.com>
 <CANbhV-GzM_KukbDm6qWFRfpkv==uDjXOOU-KMHqRz71ojce_rg@mail.gmail.com>
 <YtOqA7ldcJQADEE8@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YtOqA7ldcJQADEE8@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220719022652.GE12702%40telsasoft.com>
Precedence: bulk

Sorry, I meant to send this earlier..

On Sun, Jul 17, 2022 at 03:19:47PM +0900, Michael Paquier wrote:
> The second thing is test coverage.  Using a REINDEX DATABASE/SYSTEM

> +my $catalog_toast_index = $node->safe_psql('postgres',
> +	"SELECT indexrelid::regclass FROM pg_index WHERE indrelid = '$toast_table'::regclass;"
> +);
> +
> +# Set of SQL queries to cross-check the state of relfilenodes across
> +# REINDEX operations.  A set of relfilenodes is saved from the catalogs
> +# and then compared with pg_class.
> +$node->safe_psql('postgres',
> +	'CREATE TABLE toast_relfilenodes (relname regclass, relfilenode oid);');

It looks like you named the table "toast_relfilenodes", but then also store
to it data for non-toast tables.

It's also a bit weird to call the column "relname" but use it to store the
::regclass.  You later need to cast the column to text, so you may as well
store it as text, either relname or oid::regclass.

It seems like cluster.sql does this more succinctly.

-- Check that clustering sets new relfilenodes:
CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
CLUSTER clstrpart USING clstrpart_idx;
CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
SELECT relname, old.level, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY relname COLLATE "C";

> +# Save the relfilenode of a set of toast indexes, one from the catalog
> +# pg_constraint and one from the test table.  This data is used for checks
> +# after some of the REINDEX operations done below, checking if they are
> +# changed.
> +my $fetch_toast_relfilenodes = qq{SELECT c.oid::regclass, c.relfilenode
> +  FROM pg_class a
> +    JOIN pg_class b ON (a.oid = b.reltoastrelid)
> +    JOIN pg_index i on (a.oid = i.indrelid)
> +    JOIN pg_class c on (i.indexrelid = c.oid)
> +  WHERE b.oid IN ('pg_constraint'::regclass, 'test1'::regclass)};
> +# Same for relfilenodes of normal indexes.  This saves the relfilenode
> +# from a catalog of pg_constraint, and the one from the test table.
> +my $fetch_index_relfilenodes = qq{SELECT oid, relfilenode
> +  FROM pg_class
> +  WHERE relname IN ('pg_constraint_oid_index', 'test1x')};
> +my $save_relfilenodes =
> +	"INSERT INTO toast_relfilenodes $fetch_toast_relfilenodes;"
> +  . "INSERT INTO toast_relfilenodes $fetch_index_relfilenodes;";
> +
> +# Query to compare a set of relfilenodes saved with the contents of pg_class.
> +# Note that this does not join using OIDs, as CONCURRENTLY would change them
> +# when reindexing.  A filter is applied on the toast index names, even if this
> +# does not make a difference between the catalog and normal ones, the ordering
> +# based on the name is enough to ensure a fixed output.
> +my $compare_relfilenodes =
> +  qq(SELECT regexp_replace(b.relname::text, '(pg_toast.pg_toast_)\\d{4,5}(_index)', '\\1<oid>\\2'),

Why {4,5} ?

> +  CASE WHEN a.relfilenode = b.relfilenode THEN 'relfilenode is unchanged'
> +    ELSE 'relfilenode has changed' END
> +  FROM toast_relfilenodes b
> +    JOIN pg_class a ON b.relname::text = a.oid::regclass::text
> +  ORDER BY b.relname::text);
> +
> +# Save the set of relfilenodes and compare them.
> +$node->safe_psql('postgres', $save_relfilenodes);
> +$node->issues_sql_like(
> +	[ 'reindexdb', 'postgres' ],
> +	qr/statement: REINDEX DATABASE postgres;/,
> +	'SQL REINDEX run');
> +my $relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
> +is( $relnode_info, qq(pg_constraint_oid_index|relfilenode is unchanged
> +pg_toast.pg_toast_<oid>_index|relfilenode has changed
> +pg_toast.pg_toast_<oid>_index|relfilenode is unchanged
> +test1x|relfilenode has changed), 'relfilenode change after REINDEX DATABASE');
> +
> +# Re-save and run the second one.
> +$node->safe_psql('postgres',
> +	"TRUNCATE toast_relfilenodes; $save_relfilenodes");
> +$node->issues_sql_like(
> +	[ 'reindexdb', '-s', 'postgres' ],
> +	qr/statement: REINDEX SYSTEM postgres;/,
> +	'reindex system tables');
> +$relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
> +is( $relnode_info, qq(pg_constraint_oid_index|relfilenode has changed
> +pg_toast.pg_toast_<oid>_index|relfilenode is unchanged
> +pg_toast.pg_toast_<oid>_index|relfilenode has changed
> +test1x|relfilenode is unchanged), 'relfilenode change after REINDEX SYSTEM');