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:
> >
> > > 2) adding the dependency on heapam.h to heap.c makes sense because of
> > > heap_inplace_update bt it may be a bit annoying because I suspect
> > > that's a useful sanity check that the tableam stuff hasn't been
> > > bypassed
> >
> > That is not terrible. How plausible would it be to call vac_update_relstats()
> > for this, instead of reimplementing part of it?
>
> It didn't seem worth it to change its API to add boolean flags to skip
> setting some of the variables (I was originally only doing
> relfrozenxid and minmmxid). Now that I'm doing most of the variables
> maybe it makes a bit more sense.
>
> > > @@ -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.
>
> 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.
Regards,
--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/
