On Wed, Jul 03, 2024 at 04:09:54PM -0700, Noah Misch wrote:
> On Wed, Jul 03, 2024 at 06:00:00AM +0300, Alexander Lakhin wrote:
> > 29.06.2024 05:42, Noah Misch wrote:
> > > Good point, any effort on (2) would be wasted once the fixes get certified. I
> > > pushed (1). I'm attaching the rebased fix patches.
> >
> > Please look at a new anomaly, introduced by inplace110-successors-v5.patch:
> > CREATE TABLE t (i int) PARTITION BY LIST(i);
> > CREATE TABLE p1 (i int);
> > ALTER TABLE t ATTACH PARTITION p1 FOR VALUES IN (1);
> > ALTER TABLE t DETACH PARTITION p1;
> > ANALYZE t;
> >
> > triggers unexpected
> > ERROR: tuple to be updated was already modified by an operation triggered by the current command
>
> Thanks. Today, it's okay to issue heap_inplace_update() after heap_update()
> without an intervening CommandCounterIncrement().
Correction: it's not okay today. If code does that, heap_inplace_update()
mutates a tuple that is going to become invisible at CCI. The lack of CCI
yields a minor live bug in v14+. Its consequences seem to be limited to
failing to update reltuples for a partitioned table having zero partitions.
> The patch makes the CCI
> required. The ANALYZE in your example reaches this with a heap_update to set
> relhassubclass=f. I've fixed this by just adding a CCI (and adding to the
> tests in vacuum.sql).
That's still the right fix, but I've separated it into its own patch and
expanded the test. All the non-comment changes between v5 and v6 are now part
of the separate patch.
> The alternative would be to allow inplace updates on TM_SelfModified tuples.
> I can't think of a specific problem with allowing that, but I feel that would
> make system state interactions harder to reason about. It might be optimal to
> allow that in back branches only, to reduce the chance of releasing a bug like
> the one you found.
Allowing a mutation of a TM_SelfModified tuple is bad, since that tuple is
going to become dead soon. Mutating its successor could be okay. Since we'd
expect such code to be unreachable, I'm not keen carry such code. For that
scenario, I'd rather keep the error you encountered. Other opinions?