From: Antonin Houska <ah@cybertec.at>
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
    Pg Hackers <pgsql-hackers@lists.postgresql.org>,
    Robert Treat <rob@xzilla.net>
Subject: Re: Adding REPACK [concurrently]
In-reply-to: <CADzfLwXJLypkRdpwapQr+pZQnv1-NvkJ9DpzWhNwudQgirCE0Q@mail.gmail.com>
References: <88003.1769511456@localhost> <202602061452.uzu7cksrgzf4@alvherre.pgsql> <CADzfLwXJLypkRdpwapQr+pZQnv1-NvkJ9DpzWhNwudQgirCE0Q@mail.gmail.com>
Comments: In-reply-to Mihail Nikalayeu <mihailnikalayeu@gmail.com>
   message dated "Sun, 15 Feb 2026 17:03:00 +0100."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <14758.1771228028.1@localhost>
Date: Mon, 16 Feb 2026 08:47:08 +0100
Message-ID: <14759.1771228028@localhost>
Archived-At: <https://www.postgresql.org/message-id/14759.1771228028%40localhost>
Precedence: bulk

Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:

> Some feedback for v33.

Thanks again for your review!

> > else if (pg_strcasecmp(cmd, "REPACK") == 0)
> >      cmdtype = PROGRESS_COMMAND_REPACK;
> src/backend/utils/adt/pgstatfuncs.c:290
> 
> I think we need to add "CLUSTER" here too to avoid regression.

What kind of regression? There is no pg_stat_get_progress_info('CLUSTER') call
in system_views.sql.

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

> -----------
> 
> > /* Error queue. */
> > shm_mq       *error_mq;
> src/backend/commands/cluster.c:210.
> 
> Not used anywhere.

ok

> -----------
> 
> >   finish_heap_swap(old_table_oid, new_table_oid,
> >                is_system_catalog,
> >                false,       /* swap_toast_by_content */
> >                false, true, false,
> >                frozenXid, cutoffMulti,
> >                relpersistence);
> src/backend/commands/cluster.c
> 
> I think we should add comments for other boolean parameters.

Perhaps, it wouldn't hurt.

> -----------
> 
> > elog(ERROR, "Incomplete insert info.");
> > elog(ERROR, "Incomplete update info.");
> src/backend/replication/pgoutput_repack/pgoutput_repack.c:118,132
> 
> Should be non-capitalized?

ok

> -----------
> 
> > # Copyright (c) 2022-2024, PostgreSQL Global Development Group
> src/backend/replication/pgoutput_repack/meson.build
> 
> 2022-2026

ok

> -----------
> 
> > int         newxcnt = 0;
> src/backend/replication/logical/snapbuild.c:53
> 
> uint32 is better here.

This was already discussed:

https://www.postgresql.org/message-id/137668.1768235610%40localhost

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