Re: Adding REPACK [concurrently]

Mihail Nikalayeu <mihailnikalayeu(at)gmail(dot)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