Subject: Re: Adding REPACK [concurrently]

From: Mihail Nikalayeu <[email-masked]>
Date: Sat, 28 Feb 2026 16:16:00 +0100

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.

---

From: Antonin Houska <[email-masked]>
Date: Mon, 02 Mar 2026 18:23:30 +0100

Antonin Houska <[email-masked]> wrote:

> Mihail Nikalayeu <[email-masked]> wrote:
> > ------------
> > 
> I suppose you mean store_change(). Yes, I tried to free the individual chunks
> and forgot these. The next version uses a new, per-change memory context.

I realize now that I forgot to reset the context at the end of the
function. I'll fix that in the next version (which will probably be posted
rather soon).

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

---

From: Mihail Nikalayeu <[email-masked]>
Date: Sat, 7 Mar 2026 15:28:00 +0100

Hello!

On Thu, Mar 5, 2026 at 8:06 PM Antonin Houska <[email-masked]> wrote:
> This is it. One more diff added, to engage BulkInsertState.

Few more comments:

> * In case functions in the index need the active snapshot and caller
> * hasn't set one.

Looks like a stale comment.

-----------

> recheck = ExecInsertIndexTuples(iistate->rri,
>                         iistate->estate,
>                         0,
>                         index_slot,
>                         NIL, NULL);

Such code in apply_concurrent_update and apply_concurrent_insert.
AFAIU we need to call ResetPerTupleExprContext(iistate->estate); after
list_free(recheck); to avoid small per-row memory leak.

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

In find_target_tuple index_rescan is called before setting the
sk_argument - it works, but feels to be incorrect for the common case.
We should call it once entry->sk_argument is ready.

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

I put possible fixes into the attached patch.

Mikhail.