On Tue, Nov 10, 2020 at 04:10:57PM +0900, Masahiko Sawada wrote:
> On Mon, Nov 9, 2020 at 3:23 PM Greg Stark <stark@mit.edu> wrote:
> > On Mon, 9 Nov 2020 at 00:17, Noah Misch <noah@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.