Re: Adding REPACK [concurrently]

Mihail Nikalayeu <mihailnikalayeu(at)gmail(dot)com> wrote:

> 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 :)

Indeed, that may be a problem.

> 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)

I prefer not to depend on other improvements.

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

Worker makes more sense to me - the initial implementation is in 0005.

> ---
> > 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.

Good point, I missed this.

> And also upgrade before swap probably.

rebuild_relation_finish_concurrent() already does that.

> ---
> > cluster_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid)
>
> Should be check CheckSlotPermissions(); here? Aso, maybe it is worth
> mentioning in docs.

setup_logical_decoding() does that, but I'm not sure if we should really
require the REPLICATION user attribute for REPACK. I need to think about this,
perhaps ACL_MAINTAIN is enough.

> ---
> > 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.

I added test for TOAST to "injection_points" and hit a serious problem: when
applying concurrent changes to the new table, REPACK tried to delete rows from
the new one. The point is that the "swap TOAST by content" technique cannot be
used here. Fixed, thanks for this suggestion!

> > * 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.

ok, changed

> ---
> > rebuild_relation(RepackCommand cmd, Relation OldHeap, Relation index,
>
> "cmd" is not used.

Fixed (not specific to 0004).

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

Not sure if it's necessary for the MVCC-safe case, I consider it leftover from
some previous version. Removed.

> ---
> > rebuild_relation(RepackCommand cmd, Relation OldHeap, Relation index,
> > bool verbose, bool concurrent)
>
> "concurrent" is "concurrently" in definition.

Fixed.

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

ok

> ---
> > subtransacion
> typo
>

I removed the related code. It was a workaround for plan_cluster_use_sort()
not to leave locks behind. However, as REPACK (CONCURRENTLY) does not unlock
the relation anymore, this is not needed as well.

> ---
> > Should we check a the end
>
> "a" is "at"?

Removed when addressing one of the previous comments.

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

Fixed.

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

Fixed.

> ---
> > extern bool HeapTupleMVCCInserted(HeapTuple htup, Snapshot snapshot,
> > Buffer buffer);
> > extern bool HeapTupleMVCCNotDeleted(HeapTuple htup, Snapshot snapshot,
> > Buffer buffer);
>
> Looks like this from another patch.

Right, this is from the "MVCC safety part".

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

Yes, this belongs to some of the following patches of the series.

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

Fixed.

> ---
> > # Prefix the system columns with underscore as they are not allowed as column
> > # names.
>
> Should it be removed?

Done. (Belongs to the "MVCC-safety" part, where the test check xmin, xmax,
...)

> ---
> > "Failed to find target tuple"
>
> This and multiple other new error messages should start with lowercase

Fixed.

> ---
> > Copyright (c) 2012-2024, PostgreSQL Global Development Group
>
> in pgoutput_repack - maybe it is time to adjust.

Done.

> ---
> src/test/modules/injection_points/logical.conf
>
> Better to add newline

Done.

> ---
> > SELECT injection_points_detach('repack-concurrently-before-lock');
>
> Uses spaces, need to be tabs.

ok

Thanks for the review!

--
Antonin Houska
Web: https://www.cybertec-postgresql.com