agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Antonin Houska <[email protected]>
To: Mihail Nikalayeu <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: Robert Treat <[email protected]>
Subject: Re: Adding REPACK [concurrently]
Date: Mon, 16 Feb 2026 08:47:08 +0100
Message-ID: <14759.1771228028@localhost> (raw)
In-Reply-To: <CADzfLwXJLypkRdpwapQr+pZQnv1-NvkJ9DpzWhNwudQgirCE0Q@mail.gmail.com>
References: <88003.1769511456@localhost>
<[email protected]>
<CADzfLwXJLypkRdpwapQr+pZQnv1-NvkJ9DpzWhNwudQgirCE0Q@mail.gmail.com>
Mihail Nikalayeu <[email protected]> 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
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Adding REPACK [concurrently]
In-Reply-To: <14759.1771228028@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox