Re: Adding REPACK [concurrently]

Hello, Antonin!

> The changes present in WAL decoded prior the snapshot creation are not
> replayed - these changes are visible to the snapshot. (This is not really
> specific to the 0006 part.)

OK, just want to be sure it still works the same way if we build multiple
snapshots for the same slot that way.

> The current API does not seem to support changing snapshot of an
in-progress
> scan and I don't want to change that. Plus note that the current
> implementation of CLUSTER also uses SnapshotAny and then checks the
visibility
> separately. Finally, SnapshotAny is not really an expensive visibility
check,
> if it can be considered a visibility check at all.

But we will require a real check for each tuple. Including dead one,
multiple versions of the same HOT, etc.

> I've added it only for xmin. xid is valid because REPACK is executed in a
> transaction. That reminds me that PROC_IN_VACUUM should be present in
> MyProc->statusFlags. Fixed.

Yes, xid is required for repack. I think it is better to introduce a new
flag instead of PROC_IN_VACCUUM.

> > > PushActiveSnapshot(GetTransactionSnapshot());
> > GetLatestSnapshot() feels better here.
> What will then happen to code that uses GetActiveSnapshot() ?

O, I mean PushActiveSnapshot(GetLatestSnapshot())

> > Also, to correctly build a unique index - some tech from [0] is
required (building a unique index with multiple snapshots is a little bit
tricky).
> ok, I'll check your patch.

I realized building a unique index is still done with a single snapshot, so
it should be OK for that case. But still check the patch :)

> I proposed the Assert above, but still thinking about it.
Hm... Do we really need these asserts if PROC_IN_VACUUM is set? I was
proposing a way it is used for index building (to ensure nothing is
propagated into xmin).

Best regards,
Mikhail.