Re: BUG #18016: REINDEX TABLE failure

On Fri, Jul 7, 2023 at 5:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Michael Paquier <michael@paquier.xyz> writes:
> > On Thu, Jul 06, 2023 at 08:29:19PM +0000, PG Bug reporting form wrote:
> >> Given a table with a TOASTed variable length attribute, REINDEX TABLE fails
> >> to rebuild indexes when you truncate (or otherwise corrupt) relation files
> >> for both TOAST table index and a custom index on the varlena.
>
> > Could you clarify what you have done here?  Did you manipulate the
> > physical files in the data folder before running the REINDEX commands
> > you expected should work?  There are many things that can go wrong if
> > you do anything like that.
>
> I think the point of that was just to have a way to reproduce the problem
> on-demand.  I follow the argument, which is that if there's actual
> corruption in the TOAST index (for whatever reason) that might interfere
> with rebuilding the table's other indexes.

That's my understanding, as well.

This shouldn't be treated as a bug, but as a desirable improvement in
REINDEX TABLE's behaviour. Stated another way, we want REINDEX TABLE
to reindex toast tables' indexes before attempting to reindex the
table's index.

Below [1] are the commands to create the test case and reproduce the error.

I am taking a look at this; I'd like to avoid duplicate work if
someone else is looking at it, too.

Preliminary reading of the code indicates that a simple rearrangement
of the code in reindex_relation() would be sufficient to get the
desired behaviour. The code towards the bottom in that function,
protected by `if ((flags & REINDEX_REL_PROCESS_TOAST ...)` needs to be
moved to just before the `foreach(indexId, indexIds)` loop.

The only downside I see so far with the proposed change is that the
toast tables are currently reindexed after table_close() call, but
after the proposed change they'll be reindexed before that call to
close_table(). But since close_table() does not release the ShareLock
on the table that is taken at the beginning of reindex_relation(), I
don't think we'll losing anything by the proposed rearrangement of
code.

[1]:
initdb ./db/data
pg_ctl -D ./db/data -l db/server.log start
psql -d postgres

create table t1(column1 text);
create index on t1 (column1);
insert into t1 select repeat('fsdfaf', 30000);

select oid, relfilenode, relname from pg_class
    where oid >= (select oid from pg_class where relname = 't1');

// Generate command to corrupt toast table's index
select 'echo > db/data/base/'
|| (select oid from pg_database where datname = current_database())
|| '/'
|| (select relfilenode from pg_class
    where relname = ('pg_toast_'
        || (select oid from pg_class where relname = 't1'))
        || '_index');

# Stop the database before inducing corruption; else the reindex command may
# use cached copies of toast index blocks and succeed
pg_ctl -D ./db/data stop
echo > db/data/base/5/16388
pg_ctl -D ./db/data -l db/server.log start
psql -d postgres

reindex table t1;
ERROR:  could not read block 0 in file "base/5/16388": read only 1 of 8192 bytes

reindex index pg_toast.pg_toast_16384_index ;
//REINDEX
reindex table t1;
//REINDEX

Best regards,
Gurjeet
http://Gurje.et