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
