Re: Adding REPACK [concurrently]

Hello, comments so far on 0004:

---
> ind_oids_new = build_new_indexes(NewHeap, OldHeap, ind_oids_old);

I think the biggest issue we have so far -
repack_decode_concurrent_changes is not called while new indexes are
built (the build itself creates a huge amount of WAL and takes days
sometimes). Looks like a way to catastrophic scenarios :)

Some small parts of it may be related to reset snapshots tech in CIC case:
1) if we build new indexes concurrently in REPACK case
2) and reset snapshots every so often
3) we may use the same callback to also process WAL every so often
4) but it still not applies to some phases of index building (batch
insertion phase, for example)

Or should we move repack_decode_concurrent_changes calls into some
kind of worker instead?

---
> if (OldHeap->rd_rel->reltoastrelid)
> LockRelationOid(OldHeap->rd_rel->reltoastrelid, AccessExclusiveLock);

I think we should pass mode from rebuild_relation here - because
AccessExclusiveLock will break "CONCURRENTLY" totally.
And also upgrade before swap probably.

---
> cluster_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid)

Should be check CheckSlotPermissions(); here? Aso, maybe it is worth
mentioning in docs.

---
> REPACK (CONCURRENTLY) repack_test USING INDEX repack_test_pkey;

Some paths (without index) are not covered in any way in tests at the moment.
Also, I think some TOAST-related scenarios too.

> * Alternatively, we can lock all the indexes now in a mode that blocks
> * all the ALTER INDEX commands (ShareUpdateExclusiveLock ?), and keep

I think it's better to lock.

---
> rebuild_relation(RepackCommand cmd, Relation OldHeap, Relation index,

"cmd" is not used.

---
> apply_concurrent_update
> apply_concurrent_delete
> apply_concurrent_insert

"change" is not used, but I think it is intentionally for the MVCC-safe case.

---
> rebuild_relation(RepackCommand cmd, Relation OldHeap, Relation index,
> bool verbose, bool concurrent)

"concurrent" is "concurrently" in definition.

---

> TM_FailureData *tmfd, bool changingPart,
> bool wal_logical);
Maybe "walLogical" to keep it aligned with "changingPart"?

---
> subtransacion
typo

---
> Should we check a the end

"a" is "at"?

---
> Note that <command>REPACK</command> with the
> the <literal>CONCURRENTLY</literal> option does not try to order the

double "the"

---
> if (size >= 0x3FFFFFFF)
if (size >= MaxAllocSize)

---
> extern bool HeapTupleMVCCInserted(HeapTuple htup, Snapshot snapshot,
> Buffer buffer);
> extern bool HeapTupleMVCCNotDeleted(HeapTuple htup, Snapshot snapshot,
> Buffer buffer);

Looks like this from another patch.

---
src/backend/utils/cache/relcache.c
> #include "commands/cluster.h"

may be removed

---
> during any of the preceding
> phase.

"phases"

---
> # Prefix the system columns with underscore as they are not allowed as column
> # names.

Should it be removed?

---
> "Failed to find target tuple"

This and multiple other new error messages should start with lowercase

---
> Copyright (c) 2012-2024, PostgreSQL Global Development Group

in pgoutput_repack - maybe it is time to adjust.

---
src/test/modules/injection_points/logical.conf

Better to add newline

---
> SELECT injection_points_detach('repack-concurrently-before-lock');

Uses spaces, need to be tabs.

Next step in my plan - rebase MVCC-safe commit and test it with some
amount of stress tests.

Best regards,
Mikhail.