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.
