Обсуждение: BUG #18363: Assert !ReindexIsProcessingIndex falsified with expression index over select from table

The following bug has been logged on the website:

Bug reference:      18363
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 16.2
Operating system:   Ubuntu 22.04
Description:

The following script:
CREATE TABLE t(i INT PRIMARY KEY);
CREATE FUNCTION f(i int) RETURNS int IMMUTABLE LANGUAGE SQL
  RETURN (SELECT i FROM t WHERE i = $1);
CREATE INDEX ON t(f(i));
INSERT INTO t VALUES (1);
REINDEX INDEX t_f_idx;

triggers an assertion failure with the stack trace:
TRAP: failed
Assert("!ReindexIsProcessingIndex(RelationGetRelid(indexRelation))"), File:
"indexam.c", Line: 778, PID: 1339223
ExceptionalCondition at assert.c:52:13
index_getprocid at indexam.c:817:1
get_relation_info at plancat.c:272:24
build_simple_rel at relnode.c:379:5
add_base_rels_to_query at initsplan.c:166:10
add_base_rels_to_query at initsplan.c:173:3
query_planner at planmain.c:180:2
grouping_planner at planner.c:1495:17
subquery_planner at planner.c:1070:2
make_subplan at subselect.c:221:12
process_sublinks_mutator at subselect.c:1950:10
expression_tree_mutator_impl at nodeFuncs.c:3302:5
process_sublinks_mutator at subselect.c:2052:9
expression_tree_mutator_impl at nodeFuncs.c:3389:12
process_sublinks_mutator at subselect.c:2052:9
SS_process_sublinks at subselect.c:1924:1
preprocess_expression at planner.c:1171:10
subquery_planner at planner.c:810:20
standard_planner at planner.c:413:9
planner at planner.c:282:9
pg_plan_query at postgres.c:904:9
init_execution_state at functions.c:497:12
init_sql_fcache at functions.c:797:21
fmgr_sql at functions.c:1097:40
ExecInterpExpr at execExprInterp.c:735:7
ExecInterpExprStillValid at execExprInterp.c:1871:1
MemoryContextSwitchTo at palloc.h:142:23
 (inlined by) ExecEvalExprSwitchContext at executor.h:356:2
 (inlined by) FormIndexDatum at index.c:2774:13
heapam_index_build_range_scan at heapam_handler.c:1660:7
table_index_build_scan at tableam.h:1781:9
 (inlined by) _bt_spools_heapscan at nbtsort.c:483:15
btbuild at nbtsort.c:329:14
index_build at index.c:3042:10
reindex_index at index.c:3763:2
ReindexIndex at indexcmds.c:2793:1
ExecReindex at indexcmds.c:2741:1
standard_ProcessUtility at utility.c:965:4
ProcessUtility at utility.c:530:3
PortalRunUtility at pquery.c:1168:2
PortalRunMulti at pquery.c:1315:5
PortalRun at pquery.c:795:5
exec_simple_query at postgres.c:1282:3
PostgresMain at postgres.c:4641:27
report_fork_failure_to_client at postmaster.c:4242:1
BackendStartup at postmaster.c:4199:22
ServerLoop at postmaster.c:1788:6
BackgroundWorkerInitializeConnection at postmaster.c:5604:1
main at main.c:185:3

With a non-assertion-enabled build it fails with:
ERROR:  could not read block 0 in file "base/16384/16392": read only 0 of
8192 bytes
CONTEXT:  SQL function "f" during startup

Surely, such an index will not work correctly anyway, but may be it makes
sense to replace that Assert with ereport(ERROR).

PG Bug reporting form <noreply@postgresql.org> writes:
> The following script:
> CREATE TABLE t(i INT PRIMARY KEY);
> CREATE FUNCTION f(i int) RETURNS int IMMUTABLE LANGUAGE SQL
>   RETURN (SELECT i FROM t WHERE i = $1);
> CREATE INDEX ON t(f(i));
> INSERT INTO t VALUES (1);
> REINDEX INDEX t_f_idx;

> triggers an assertion failure with the stack trace:
> ...

> With a non-assertion-enabled build it fails with:
> ERROR:  could not read block 0 in file "base/16384/16392": read only 0 of
> 8192 bytes
> CONTEXT:  SQL function "f" during startup

> Surely, such an index will not work correctly anyway, but may be it makes
> sense to replace that Assert with ereport(ERROR).

Yeah, I guess so.  We periodically get reports of the non-assert
failure, and this would let us issue a more on-point error message.
I'm slightly worried about the extra cost, but typically
pendingReindexedIndexes should be empty or at least short, so it's
probably negligible.

            regards, tom lane

On Sun, Feb 25, 2024 at 03:37:08PM -0500, Tom Lane wrote:
> Yeah, I guess so.  We periodically get reports of the non-assert
> failure, and this would let us issue a more on-point error message.
> I'm slightly worried about the extra cost, but typically
> pendingReindexedIndexes should be empty or at least short, so it's
> probably negligible.

An index expression that calls a function doing a scan of its parent
table is a funky case for sure, but perhaps f5a465f1a074 would have
been better with a test?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> An index expression that calls a function doing a scan of its parent
> table is a funky case for sure, but perhaps f5a465f1a074 would have
> been better with a test?

I wasn't excited about having a test case, but feel free to
add one if you think it's worth the cycles.

            regards, tom lane

Hello Tom and Michael,

25.02.2024 23:37, Tom Lane wrote:
>> Surely, such an index will not work correctly anyway, but may be it makes
>> sense to replace that Assert with ereport(ERROR).
> Yeah, I guess so.  We periodically get reports of the non-assert
> failure, and this would let us issue a more on-point error message.
> I'm slightly worried about the extra cost, but typically
> pendingReindexedIndexes should be empty or at least short, so it's
> probably negligible.

Thank you for fixing that!

I've also got the same error (but not the assert) with CREATE INDEX:
CREATE TABLE t(i int PRIMARY KEY);
CREATE FUNCTION f(c int) RETURNS INT IMMUTABLE LANGUAGE SQL
     AS 'SELECT i FROM t WHERE i = $1';
INSERT INTO t VALUES (1);
CREATE INDEX ON t(f(i));

ERROR:  could not read block 0 in file "base/16384/16391": read only 0 of 8192 bytes
CONTEXT:  SQL function "f" during startup

It looks like currentlyReindexedIndex == 0 in this case, so
ReindexIsProcessingIndex() doesn't guard against get_relation_info() ->
_bt_getrootheight() -> _bt_getbuf() -> ReadBuffer() -> ... -> mdread().

Best regards,
Alexander

On Mon, Feb 26, 2024 at 09:00:01AM +0300, Alexander Lakhin wrote:
> It looks like currentlyReindexedIndex == 0 in this case, so
> ReindexIsProcessingIndex() doesn't guard against get_relation_info() ->
> _bt_getrootheight() -> _bt_getbuf() -> ReadBuffer() -> ... -> mdread().

Fun.  The concurrent case is actually able to work because it looks
like the basic definition of the relation exists with its relfilenode.

I was looking at that, and it seems to me that the point is not that
the index is being reindexed; the point is that we want to prevent
access to the index itself while it being built.  And that's something
that can happen for a reindex as much as a new index.  It would be
possible to paint an equivalent of SetReindexProcessing() in
index_create() for index_build() where a trace of the index OID
getting built is kept around, and it would be possible to trigger the
same  error as when doing a reindex.  Hence, if we were to do that,
the current ReindexIs*() routines maintaining the list of the indexes
being built across transaction states are a bit misnamed, and the
error messages would be partially incorrect.

I am not sure if this is worth bothering beyond HEAD, or worth
bothering at all, but seeing 940489b467 it looks like we do bother
even for stable branches.
--
Michael