pgstatindex vs. !indisready
Running pgstatindex on some !indisready indexes fails with "ERROR: XX001:
could not read block 0 in file". This reproduces it:
begin;
drop table if exists not_indisready_t;
create table not_indisready_t (c int);
insert into not_indisready_t values (1),(1);
commit;
create unique index concurrently not_indisready_i on not_indisready_t(c);
begin;
create extension pgstattuple;
\set VERBOSITY verbose
select * from pgstatindex('not_indisready_i');
\set VERBOSITY default
rollback;
Since XX001 = ERRCODE_DATA_CORRUPTED appears in the "can't-happen" class, it's
not a good fit for this scenario. I propose to have pgstatindex fail early on
!indisready indexes. We could go a step further and also fail on
indisready&&!indisvalid indexes, which are complete enough to accept inserts
but not complete enough to answer queries. I don't see a reason to do that,
but maybe someone else will.
This made me wonder about functions handling unlogged rels during recovery. I
used the attached hack to test most regclass-arg functions. While some of
these errors from src/test/recovery/tmp_check/log/001_stream_rep_standby_2.log
may deserve improvement, there were no class-XX messages:
2023-10-01 12:24:05.992 PDT [646914:11] 001_stream_rep.pl ERROR: 58P01: could
not open file "base/5/16862": No such file or directory
2023-10-01 12:24:05.996 PDT [646914:118] 001_stream_rep.pl ERROR: 22023: fork
"main" does not exist for this relation
Thanks,
nm
Author: Noah Misch
Commit: Noah Misch
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index c60314d..ad54859 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -10,6 +10,14 @@
#-
EXTRA_INSTALL=contrib/pg_prewarm contrib/pg_stat_statements
contrib/test_decoding
+EXTRA_INSTALL += \
+ contrib/pg_visibility \
+ contrib/amcheck \
+ contrib/pgstattuple \
+ contrib/btree_gin \
+ contrib/pg_surgery \
+ contrib/pg_freespacemap \
+ contrib/pgrowlocks
subdir = src/test/recovery
top_builddir = ../../..
diff --git a/src/test/recovery/t/001_stream_rep.pl
b/src/test/recovery/t/001_stream_rep.pl
index 0c72ba0..e441cb2 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -45,6 +45,24 @@ $node_standby_2->start;
# Create some content on primary and check its presence in standby nodes
$node_primary->safe_psql('postgres',
"CREATE TABLE tab_int AS SELECT generate_series(1,1002) AS a");
+$node_primary->safe_psql('postgres', "
+create extension pg_visibility;
+create extension amcheck;
+create extension pgstattuple;
+create extension btree_gin;
+create extension pg_surgery;
+create extension pg_prewarm;
+create extension pg_freespacemap;
+create extension pgrowlocks;
+
+create unlogged sequence useq;
+create unlogged table u (c text);
+insert into u values ('foo');
+create index ubtree on u (c);
+create index ugin on u using gin (c);
+create index uhash on u using hash (c);
+create index ubrin on u using brin (c);
+");
# Wait for standbys to catch up
$node_primary->wait_for_replay_catchup($node_standby_1);
@@ -60,6 +78,88 @@ $result =
print "standby 2: $result\n";
is($result, qq(1002), 'check streamed content on standby 2');
+# Call regclass-arg functions on unlogged objects. Function list from:
+#
+# DO $$
+# DECLARE
+# extname text;
+# BEGIN
+# FOR extname in SELECT name from pg_available_extensions LOOP
+# EXECUTE format('CREATE EXTENSION IF NOT EXISTS %I CASCADE',
extname);
+# END LOOP;
+# END
+# $$;
+# select oid::regprocedure from pg_proc
+# where proargtypes::oid[] @> array['regclass'::regtype::oid]
+# order by oid::regprocedure::text;
+#
+# Removed pageinspect functions, since the superuser might choose to run them
+# in cases we wouldn't expect. Added pgrowlocks(text).
+$node_standby_2->psql('postgres', <<'EOSQL', on_error_stop => 0);
+\set utable '''u''::regclass'
+\set VERBOSITY verbose
+SET log_error_verbosity = verbose;
+SET log_min_messages = debug1; -- for amcheck
+SET client_min_messages = debug1;
+
+SELECT * FROM pgrowlocks(:utable::text);
+SELECT brin_desummarize_range('ubrin',1);
+SELECT brin_summarize_new_values('ubrin');
+SELECT brin_summarize_range('ubrin',1);
+SELECT bt_index_check('ubtree');
+SELECT bt_index_check('ubtree',true);
+SELECT bt_index_parent_check('ubtree');
+SELECT bt_index_parent_check('ubtree',true);
+SELECT bt_index_parent_check('ubtree',true,true);
+SELECT gin_clean_pending_list('ugin');
+SELECT heap_force_freeze(:utable,array['(0,1)'::tid]);
+SELECT heap_force_kill(:utable,array['(0,1)'::tid]);
+SELECT nextval('useq');
+SELECT currval('useq');
+SELECT pg_check_frozen(:utable);
+SELECT pg_check_visible(:utable);
+SELECT pg_column_is_updatable(:utable,'1',true);
+--SELECT pg_extension_config_dump(:utable,'select 1');
+SELECT pg_freespace(:utable);
+SELECT pg_freespace(:utable,1);
+SELECT pg_get_replica_identity_index(:utable);
Re: pgstatindex vs. !indisready
On Wed, Oct 04, 2023 at 09:00:23AM +0900, Michael Paquier wrote:
> On Sun, Oct 01, 2023 at 06:31:26PM -0700, Noah Misch wrote:
> > The !indisvalid index may be missing tuples, yes. In what way does that
> > make
> > one of those four operations incorrect?
>
> Reminding myself of what these four do, it looks that you're right and
> that the state of indisvalid is not going to change what they report.
>
> Still, I'd like to agree with Tom's point to be more conservative and
> check also for indisvalid which is what the planner does. These
> functions will be used in SELECTs, and one thing that worries me is
> that checks based on indisready may get copy-pasted somewhere else,
> leading to incorrect results where they get copied. (indisready &&
> !indisvalid) is a "short"-term combination in a concurrent build
> process, as well (depends on how long one waits for the old snapshots
> before switching indisvalid, but that's way shorter than the period of
> time where the built indexes remain valid).
Neither choice would harm the user experience in an important way, so I've
followed your preference in the attached patch.
Author: Noah Misch
Commit: Noah Misch
Diagnose !indisvalid in more SQL functions.
pgstatindex failed with ERRCODE_DATA_CORRUPTED, of the "can't-happen"
class XX. The other functions succeeded on an empty index; they might
have malfunctioned if the failed index build left torn I/O or other
complex state. Report an ERROR in statistics functions pgstatindex,
pgstatginindex, pgstathashindex, and pgstattuple. Report DEBUG1 and
skip all index I/O in maintenance functions brin_desummarize_range,
brin_summarize_new_values, brin_summarize_range, and
gin_clean_pending_list. Back-patch to v11 (all supported versions).
Reviewed by FIXME.
Discussion: https://postgr.es/m/20231001195309...@google.com
diff --git a/contrib/pgstattuple/pgstatindex.c
b/contrib/pgstattuple/pgstatindex.c
index d69ac1c..8e5a4d6 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -238,6 +238,18 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
errmsg("cannot access temporary tables of
other sessions")));
/*
+* A !indisready index could lead to ERRCODE_DATA_CORRUPTED later, so
exit
+* early. We're capable of assessing an indisready&&!indisvalid index,
+* but the results could be confusing. For example, the index's size
+* could be too low for a valid index of the table.
+*/
+ if (!rel->rd_index->indisvalid)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("index \"%s\" is not valid",
+ RelationGetRelationName(rel;
+
+ /*
* Read metapage
*/
{
@@ -523,6 +535,13 @@ pgstatginindex_internal(Oid relid, FunctionCallInfo fcinfo)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary indexes of
other sessions")));
+ /* see pgstatindex_impl */
+ if (!rel->rd_index->indisvalid)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("index \"%s\" is not valid",
+ RelationGetRelationName(rel;
+
/*
* Read metapage
*/
@@ -600,6 +619,13 @@ pgstathashindex(PG_FUNCTION_ARGS)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary indexes of
other sessions")));
+ /* see pgstatindex_impl */
+ if (!rel->rd_index->indisvalid)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("index \"%s\" is not valid",
+ RelationGetRelationName(rel;
+
/* Get the information we need from the metapage. */
memset(&stats, 0, sizeof(stats));
metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
diff --git a/contrib/pgstattuple/pgstattuple.c
b/contrib/pgstattuple/pgstattuple.c
index 93b7834..3bd8b96 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -259,6 +259,13 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
}
else if (rel->rd_rel->relkind == RELKIND_INDEX)
{
+ /* see pgstatindex_impl */
+ if (!rel->rd_index->indisvalid)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("index \"%s\" is n
Re: pgstatindex vs. !indisready
On Sun, Oct 22, 2023 at 02:02:45PM -0700, Noah Misch wrote:
> - /* OK, do it */
> - brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
> + /* see gin_clean_pending_list() */
> + if (indexRel->rd_index->indisvalid)
> + brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized,
> NULL);
> + else
> + ereport(DEBUG1,
> +
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("index \"%s\" is not valid",
> +
> RelationGetRelationName(indexRel;
brinsummarize() could return 0 even for a valid index, and we would
not be able to make the difference with an invalid index. Perhaps you
are right and this is not a big deal in practice to do as you are
suggesting.
--
Michael
signature.asc
Description: PGP signature
Re: pgstatindex vs. !indisready
Noah Misch writes: > Running pgstatindex on some !indisready indexes fails with "ERROR: XX001: > could not read block 0 in file". This reproduces it: > ... > Since XX001 = ERRCODE_DATA_CORRUPTED appears in the "can't-happen" class, it's > not a good fit for this scenario. I propose to have pgstatindex fail early on > !indisready indexes. +1 > We could go a step further and also fail on > indisready&&!indisvalid indexes, which are complete enough to accept inserts > but not complete enough to answer queries. I don't see a reason to do that, > but maybe someone else will. Hmm. Seems like the numbers pgstatindex would produce for a not-yet-complete index would be rather irrelevant, even if the case doesn't risk any outright problems. I'd be inclined to be conservative and insist on indisvalid being true too. regards, tom lane
Re: pgstatindex vs. !indisready
On Sun, Oct 01, 2023 at 04:37:25PM -0400, Tom Lane wrote:
> Noah Misch writes:
> > Running pgstatindex on some !indisready indexes fails with "ERROR: XX001:
> > could not read block 0 in file". This reproduces it:
> > ...
> > Since XX001 = ERRCODE_DATA_CORRUPTED appears in the "can't-happen" class,
> > it's
> > not a good fit for this scenario. I propose to have pgstatindex fail early
> > on
> > !indisready indexes.
>
> +1
>
> > We could go a step further and also fail on
> > indisready&&!indisvalid indexes, which are complete enough to accept inserts
> > but not complete enough to answer queries. I don't see a reason to do that,
> > but maybe someone else will.
>
> Hmm. Seems like the numbers pgstatindex would produce for a
> not-yet-complete index would be rather irrelevant, even if the case
> doesn't risk any outright problems. I'd be inclined to be
> conservative and insist on indisvalid being true too.
Okay. If no other preferences appear, I'll do pgstatindex that way.
> > This made me wonder about functions handling unlogged rels during recovery.
> > I
> > used the attached hack to test most regclass-arg functions.
I forgot to test the same battery of functions on !indisready indexes. I've
now done that, using the attached script. While I didn't get additional
class-XX errors, more should change:
[pgstatginindex pgstathashindex pgstattuple] currently succeed. Like
pgstatindex, they should ERROR, including in the back branches.
[brin_desummarize_range brin_summarize_new_values brin_summarize_range
gin_clean_pending_list] currently succeed. I propose to make them emit a
DEBUG1 message and return early, like amcheck does, except on !indisready.
This would allow users to continue running them on all indexes of the
applicable access method. Doing these operations on an
indisready&&!indisvalid index is entirely reasonable, since they relate to
INSERT/UPDATE/DELETE operations.
[pg_freespace pg_indexes_size pg_prewarm] currently succeed, and I propose to
leave them that way. No undefined behavior arises. pg_freespace needs to be
resilient to garbage data anyway, given the lack of WAL for the FSM. One
could argue for a relkind check in pg_indexes_size. One could argue for
treating pg_prewarm like amcheck (DEBUG1 and return).
rollback;
begin;
create extension if not exists btree_gin;
create or replace function error(text) returns text language plpgsql as $$
begin
raise exception 'break index build';
return $1;
end;
$$ immutable;
drop table if exists u;
create table u (c text);
insert into u values ('foo');
commit;
create index concurrently ubtree on u (error(c));
create index concurrently ugin on u using gin (error(c));
create index concurrently uhash on u using hash (error(c));
create index concurrently ubrin on u using brin (error(c));
\set utable '''ubtree''::regclass'
\set useq '''ubtree''::regclass'
\set VERBOSITY verbose
begin;
create or replace function error(text) returns text language sql
as 'select $1' immutable;
SET log_error_verbosity = verbose;
SET log_min_messages = debug1; -- for amcheck
--SET client_min_messages = debug1;
create extension pg_visibility;
create extension amcheck;
create extension pgstattuple;
create extension pg_surgery;
create extension pg_prewarm;
create extension pg_freespacemap;
create extension pgrowlocks;
SELECT * FROM pgrowlocks(:utable::text);
SELECT brin_desummarize_range('ubrin',1);
SELECT brin_summarize_new_values('ubrin');
SELECT brin_summarize_range('ubrin',1);
SELECT bt_index_check('ubtree');
SELECT bt_index_check('ubtree',true);
SELECT bt_index_parent_check('ubtree');
SELECT bt_index_parent_check('ubtree',true);
SELECT bt_index_parent_check('ubtree',true,true);
SELECT gin_clean_pending_list('ugin');
SELECT heap_force_freeze(:utable,array['(0,1)'::tid]);
SELECT heap_force_kill(:utable,array['(0,1)'::tid]);
SELECT nextval(:useq);
SELECT currval(:useq);
SELECT pg_check_frozen(:utable);
SELECT pg_check_visible(:utable);
SELECT pg_column_is_updatable(:utable,'1',true);
--SELECT pg_extension_config_dump(:utable,'select 1');
SELECT pg_freespace(:utable);
SELECT pg_freespace(:utable,1);
SELECT pg_get_replica_identity_index(:utable);
SELECT pg_index_column_has_property(:utable,1,'asc');
SELECT pg_indexes_size(:utable);
SELECT pg_index_has_property('ubrin','asc');
--SELECT pg_nextoid(:utable,name,:utable);
SELECT pg_partition_ancestors(:utable);
SELECT pg_partition_root(:utable);
SELECT pg_partition_tree(:utable);
SELECT pg_prewarm(:utable);
SELECT pg_relation_filenode(:utable);
SELECT pg_relation_filepath(:utable);
SELECT pg_relation_is_publishable(:utable);
SELECT pg_relation_is_updatable(:utable,true);
SELECT pg_relation_size(:utable);
SELECT pg_relation_size(:utable,'main');
SELECT pg_relpages(:utable);
SELECT pg_sequence_last_value(:useq);
SELECT pgstatginindex('ugin');
SELECT pgstathashindex('uhash');
SELECT pgstatindex('ubtree');
SELECT pgstattuple_approx(:utable);
SELECT pgstattuple(:utable);
SELECT pg_table_size(:
Re: pgstatindex vs. !indisready
On Sun, Oct 1, 2023 at 2:00 PM Noah Misch wrote: > Okay. If no other preferences appear, I'll do pgstatindex that way. Sounds reasonable. > [pgstatginindex pgstathashindex pgstattuple] currently succeed. Like > pgstatindex, they should ERROR, including in the back branches. Makes sense. > [brin_desummarize_range brin_summarize_new_values brin_summarize_range > gin_clean_pending_list] currently succeed. I propose to make them emit a > DEBUG1 message and return early, like amcheck does, except on !indisready. > This would allow users to continue running them on all indexes of the > applicable access method. Doing these operations on an > indisready&&!indisvalid index is entirely reasonable, since they relate to > INSERT/UPDATE/DELETE operations. +1 to all that (including the part about these operations being a little different to the amcheck functions in one particular respect). FWIW, amcheck's handling of unlogged relations when in hot standby mode is based on the following theory: if recovery were to end, the index would become an empty index, so just treat it as if it was already empty during recovery. Not everybody thought that this behavior was ideal, but ISTM that it has fewer problems than any alternative approach you can think of. The same argument works just as well with any function that accepts a regclass argument IMV. -- Peter Geoghegan
Re: pgstatindex vs. !indisready
On Sun, Oct 01, 2023 at 04:20:42PM -0700, Peter Geoghegan wrote: > On Sun, Oct 1, 2023 at 2:00 PM Noah Misch wrote: > > Okay. If no other preferences appear, I'll do pgstatindex that way. > > Sounds reasonable. > >> [pgstatginindex pgstathashindex pgstattuple] currently succeed. Like >> pgstatindex, they should ERROR, including in the back branches. > > Makes sense. These are already restrictive, makes sense. >> [brin_desummarize_range brin_summarize_new_values brin_summarize_range >> gin_clean_pending_list] currently succeed. I propose to make them emit a >> DEBUG1 message and return early, like amcheck does, except on !indisready. >> This would allow users to continue running them on all indexes of the >> applicable access method. Doing these operations on an >> indisready&&!indisvalid index is entirely reasonable, since they relate to >> INSERT/UPDATE/DELETE operations. Hmm. Still slightly incorrect in some cases? Before being switched to indisvalid, an index built concurrently may miss some tuples deleted before the reference snapshot used to build the index was taken. > +1 to all that (including the part about these operations being a > little different to the amcheck functions in one particular respect). Making them return early sounds sensible here. > FWIW, amcheck's handling of unlogged relations when in hot standby > mode is based on the following theory: if recovery were to end, the > index would become an empty index, so just treat it as if it was > already empty during recovery. Not everybody thought that this > behavior was ideal, but ISTM that it has fewer problems than any > alternative approach you can think of. The same argument works just as > well with any function that accepts a regclass argument IMV. It depends, I guess, on how "user-friendly" all that should be. I have seen in the past as argument that it may be sometimes better to have a function do nothing rather than ERROR when these are used across a scan of pg_class, for example, particularly for non-SRFs. Even if sometimes errors can be bypassed with more quals on the relkind or such (aka less complicated queries with less JOINs to write). -- Michael signature.asc Description: PGP signature
Re: pgstatindex vs. !indisready
On Sun, Oct 1, 2023 at 5:24 PM Michael Paquier wrote: > > FWIW, amcheck's handling of unlogged relations when in hot standby > > mode is based on the following theory: if recovery were to end, the > > index would become an empty index, so just treat it as if it was > > already empty during recovery. Not everybody thought that this > > behavior was ideal, but ISTM that it has fewer problems than any > > alternative approach you can think of. The same argument works just as > > well with any function that accepts a regclass argument IMV. > > It depends, I guess, on how "user-friendly" all that should be. I > have seen in the past as argument that it may be sometimes better to > have a function do nothing rather than ERROR when these are used > across a scan of pg_class, for example, particularly for non-SRFs. > Even if sometimes errors can be bypassed with more quals on the > relkind or such (aka less complicated queries with less JOINs to > write). I think of recovery as a property of the whole system. So throwing an error about one particular unlogged index that the user (say) checked during recovery doesn't seem sensible. After all, the answer that RecoveryInProgress() gives can change in a way that's observable within individual transactions. Again, I wouldn't claim that this is very elegant. Just that it seems to have the fewest problems. -- Peter Geoghegan
Re: pgstatindex vs. !indisready
On Mon, Oct 02, 2023 at 09:24:33AM +0900, Michael Paquier wrote: > On Sun, Oct 01, 2023 at 04:20:42PM -0700, Peter Geoghegan wrote: > > On Sun, Oct 1, 2023 at 2:00 PM Noah Misch wrote: > >> [brin_desummarize_range brin_summarize_new_values brin_summarize_range > >> gin_clean_pending_list] currently succeed. I propose to make them emit a > >> DEBUG1 message and return early, like amcheck does, except on !indisready. > >> This would allow users to continue running them on all indexes of the > >> applicable access method. Doing these operations on an > >> indisready&&!indisvalid index is entirely reasonable, since they relate to > >> INSERT/UPDATE/DELETE operations. > > Hmm. Still slightly incorrect in some cases? Before being switched > to indisvalid, an index built concurrently may miss some tuples > deleted before the reference snapshot used to build the index was > taken. The !indisvalid index may be missing tuples, yes. In what way does that make one of those four operations incorrect?
Re: pgstatindex vs. !indisready
On Sun, Oct 01, 2023 at 06:31:26PM -0700, Noah Misch wrote: > The !indisvalid index may be missing tuples, yes. In what way does that make > one of those four operations incorrect? Reminding myself of what these four do, it looks that you're right and that the state of indisvalid is not going to change what they report. Still, I'd like to agree with Tom's point to be more conservative and check also for indisvalid which is what the planner does. These functions will be used in SELECTs, and one thing that worries me is that checks based on indisready may get copy-pasted somewhere else, leading to incorrect results where they get copied. (indisready && !indisvalid) is a "short"-term combination in a concurrent build process, as well (depends on how long one waits for the old snapshots before switching indisvalid, but that's way shorter than the period of time where the built indexes remain valid). -- Michael signature.asc Description: PGP signature
