Horton

Thread

Commits

  1. Promote assertion about !ReindexIsProcessingIndex to runtime error.

  1. BUG #18363: Assert !ReindexIsProcessingIndex falsified with expression index over select from table

    The Post Office <noreply@postgresql.org> — 2024-02-25T18:00:01Z

    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).

  2. Re: BUG #18363: Assert !ReindexIsProcessingIndex falsified with expression index over select from table

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-25T20:37:08Z

    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

  3. Re: BUG #18363: Assert !ReindexIsProcessingIndex falsified with expression index over select from table

    Michael Paquier <michael@paquier.xyz> — 2024-02-25T22:57:34Z

    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

  4. Re: BUG #18363: Assert !ReindexIsProcessingIndex falsified with expression index over select from table

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-25T23:04:58Z

    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

  5. Re: BUG #18363: Assert !ReindexIsProcessingIndex falsified with expression index over select from table

    Alexander Law <exclusion@gmail.com> — 2024-02-26T06:00:01Z

    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

  6. Re: BUG #18363: Assert !ReindexIsProcessingIndex falsified with expression index over select from table

    Michael Paquier <michael@paquier.xyz> — 2024-02-29T05:46:07Z

    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