Re: Adding REPACK [concurrently]

Hello!

Some feedback for v33.

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

-----------

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

-----------

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

Not used anywhere.

-----------

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

-----------

> elog(ERROR, "Incomplete insert info.");
> elog(ERROR, "Incomplete update info.");
src/backend/replication/pgoutput_repack/pgoutput_repack.c:118,132

Should be non-capitalized?

-----------

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

2022-2026

-----------

> int newxcnt = 0;
src/backend/replication/logical/snapbuild.c:53

uint32 is better here.

Best regards,
Mikhail.