Re: Adding REPACK [concurrently]

Hello!

Some review comments:

------------

> attrs = palloc0_array(Datum, desc->natts);
> isnull = palloc0_array(bool, desc->natts);

It looks like there is a memory leak with those arrays.

------------

> # TOAST pointer, wich we need to update
typo

------------

> ident_idx = RelationGetReplicaIndex(rel);
> if (!OidIsValid(ident_idx) && OidIsValid(rel->rd_pkindex))

check_repack_concurrently_requirements uses rd_pkindex as fallback.

But rebuild_relation_finish_concurrent does not contain such logic:

> ident_idx_old = RelationGetReplicaIndex(OldHeap);

------------

> >
> > > ConditionVariablePrepareToSleep(&shared->cv);
> > > for (;;)
> > > {
> > > bool initialized;
> > >
> > > SpinLockAcquire(&shared->mutex);
> > > initialized = shared->initialized;
> > > SpinLockRelease(&shared->mutex);
> > src/backend/commands/cluster.c:3663
> >
> > I think we should check GetBackgroundWorkerPid for worker status, to
> > not wait forever in case of some issue with the worker.

> ConditionVariableSleep() calls CHECK_FOR_INTERRUPTS(). That should process
> error messages from the worker.

Hm, yes, and RepackWorkerShutdown will detach the queue. But
ProcessRepackMessages does not react somehow to SHM_MQ_DETACHED - just
ignores. Or am I missing something?
And looks like it applies to all wait-loops related to repack.

------------

> build_identity_key
> ....
> n = ident_idx->indnatts;

Should we use indnkeyatts here?

------------

> build_identity_key
> ....
> entry->sk_collation = att->attcollation;

Should we use index collation (not heap) here?
entry->sk_collation = ident_idx_rel->rd_indcollation[i];

------------

> SnapBuildInitialSnapshotForRepack

What is about to add defensive checks like SnapBuildInitialSnapshot does?

> if (!builder->committed.includes_all_transactions)
> elog(ERROR, "cannot build an initial slot snapshot, not all transactions are monitored anymore");

Best regards,
Mikhail.