From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Michael Paquier <michael@paquier.xyz>,
        Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: REINDEX INDEX results in a crash for an index of pg_class since
 9.6
In-reply-to: <20190502172851.klckhiyukqd3bd2t@alap3.anarazel.de>
References: <16043.1556754084@sss.pgh.pa.us>
 <20190502012520.eyfhhn5ix2ooflys@alap3.anarazel.de>
 <27459.1556762513@sss.pgh.pa.us>
 <20190502021913.axyvyyyw5547swy5@alap3.anarazel.de>
 <25661.1556808540@sss.pgh.pa.us>
 <20190502153107.o5zadep4ehebzoqy@alap3.anarazel.de>
 <4870.1556811688@sss.pgh.pa.us>
 <20190502155612.ep5wqyninmkrz6uh@alap3.anarazel.de>
 <5531.1556812964@sss.pgh.pa.us> <2157.1556816395@sss.pgh.pa.us>
 <20190502172851.klckhiyukqd3bd2t@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Thu, 02 May 2019 10:28:51 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <10649.1556830451.1@sss.pgh.pa.us>
Date: Thu, 02 May 2019 16:54:11 -0400
Message-ID: <10650.1556830451@sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> On 2019-05-02 12:59:55 -0400, Tom Lane wrote:
>> One interesting thing that turns up in check-world is that if wal_level
>> is minimal, we have to manually force an XID to be assigned, else
>> reindexing pg_class fails with "cannot commit a transaction that deleted
>> files but has no xid" :-(.  Perhaps there's some other cleaner place to
>> do that?

> Hm. We could replace that RecordTransactionCommit() with an xid
> assignment or such. But that seems at least as fragile. Or we could
> expand the logic we have for LogStandbyInvalidations() a few lines below
> the elog to also be able to handle files.  IIRC that was introduced to
> handle somewhat related issues about being able to run VACUUM
> (containing invalidations) without an xid.

Well, that's something we can maybe improve later.  I'm content to leave
the patch as it is for now.

>> if (RelationIsMapped(relation))
>> +	{
>> +		/* Since we're not updating pg_class, these had better not change */
>> +		Assert(classform->relfrozenxid == freezeXid);
>> +		Assert(classform->relminmxid == minmulti);
>> +		Assert(classform->relpersistence == persistence);

> Hm. Could we additionally assert that we're dealing with an index?

Will do.

>> +		/* These changes are safe even for a mapped relation */

> You'd probably have noticed that, but this one probably has to go.

Ah, right.  As I said, I'd not paid much attention to the comments yet.

I just finished a successful run of the core regression tests with CCA.
Given the calendar, I think that's about as much CCA testing as I should
do personally.  I'll make a cleanup pass on this patch and try to get it
pushed within a few hours, if there are not objections.

How do you feel about the other patch to rejigger the order of operations
in CommandCounterIncrement?  I think that's a bug, but it's probably
noncritical for most people.  What I'm leaning towards for that one is
waiting till after the minor releases, then pushing it to all branches.

			regards, tom lane