On Tue, Nov 10, 2020 at 04:10:57PM +0900, Masahiko Sawada wrote: > On Mon, Nov 9, 2020 at 3:23 PM Greg Stark <st...@mit.edu> wrote: > > On Mon, 9 Nov 2020 at 00:17, Noah Misch <n...@leadboat.com> wrote:
> > > > @@ -3340,6 +3383,7 @@ heap_truncate_one_rel(Relation rel) > > > > > > > > /* Truncate the underlying relation */ > > > > table_relation_nontransactional_truncate(rel); > > > > + ResetVacStats(rel); > > > > > > I didn't test, but I expect this will cause a stats reset for the second > > > TRUNCATE here: > > > > > > CREATE TABLE t (); > > > ... > > > BEGIN; > > > TRUNCATE t; > > > TRUNCATE t; -- inplace relfrozenxid reset > > > ROLLBACK; -- inplace reset survives > > > > > > Does that indeed happen? > > > > Apparently no, see below. I have to say I was pretty puzzled by the > > actual behaviour which is that the rollback actually does roll back > > the inplace update. But I *think* what is happening is that the first > > truncate does an MVCC update so the inplace update happens only to the > > newly created tuple which is never commited. > > I think in-plase update that the patch introduces is not used because > TRUNCATE doesn't use heap_truncate_one_rel() to truncate a table in > that scenario. It does MVCC update the pg_class tuple for a new > relfilenode with new relfrozenxid and other stats, see > RelationSetNewRelfilenode(). If we create and truncate a table within > the transaction it does in-place update that the patch introduces but > I think it's no problem in this case either. Agreed. Rolling back a heap_truncate_one_rel() always implies rolling back to an earlier version of the entire pg_class tuple. (That may not be true of mapped relations, but truncating them is unreasonable.) Thanks for checking. > > Thinking about things a bit this does worry me a bit. I wonder if > > inplace update is really safe outside of vacuum where we know we're > > not in a transaction that can be rolled back. But IIRC doing a > > non-inplace update on pg_class for these columns breaks other things. > > I don't know if that's still true. > > heap_truncate_one_rel() is not a transaction-safe operation. Doing > in-place updates during that operation seems okay to me unless I'm > missing something. Yep.