agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Antonin Houska <ah@cybertec.at>
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Robert Treat <rob@xzilla.net>
Subject: Re: Adding REPACK [concurrently]
Date: Tue, 20 Jan 2026 16:39:10 +0100
Message-ID: <73630.1768923550@localhost> (raw)
In-Reply-To: <CADzfLwUJSHKGxYw+vMUZ_Hr2YeuxO2Q5w13HKgUUN1725tjY5Q@mail.gmail.com>
References: <202512151349.vlq3mpfniyk3@alvherre.pgsql>
<11247.1767609087@localhost>
<11558.1767609632@localhost>
<141054.1767891540@localhost>
<CADzfLwU-OmxW3t3AoQo9=K7uq4G1yZ-txcetzW3jbcVxV_pJew@mail.gmail.com>
<137668.1768235610@localhost>
<CADzfLwUJSHKGxYw+vMUZ_Hr2YeuxO2Q5w13HKgUUN1725tjY5Q@mail.gmail.com>
Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:
> > if (size >= MaxAllocSize)
> Seems like we lost that check, I think it may be executed on storing
> the data
The tuple we process in store_change was created elsewhere (I think in
reorderbuffer.c), so I wouldn't re-check the size here.
> or before "tup = (HeapTuple) palloc(HEAPTUPLESIZE + t_len);"
> in apply_concurrent_changes
It's essentially the same length that we write in store_change() so I wouldn't
bother re-checking it here. In general, I doubt the constant is
appropriate. Its meaning is much more generic than the size of memory for a
tuple and even heap_form_tuple() does not use it.
> > bool done;
> I still think it is a confusing name.
I don't. The last call of process_concurrent_changes() tells the worker "Give
me the the next batch and we are done". Your proposal "exit_after_lsn_upto"
seems to me too verbose: the worker itself is supposed to know that it has to
reach the LSN passed via another argument.
> > chgdst.file_seq = WORKER_FILE_SNAPSHOT + 1;
> I think it is better to increment it once a snapshot is received.
The 'chgdst' is only defined in rebuild_relation_finish_concurrent(), no need
to use it where the snapshot is received (in rebuild_relation()).
> And rename to last_processed/last_improrted to be aligned with
> last_exported.
While DecodingWorkerShared deals with multiple files (not all of them
necessarily available for "consumer") and therefore it makes sense to
distinguish if file is exported or not, each instance of ChangeDest is
assigned particular file and the functions using it do not care if the file is
the last in the sequence or not.
Other proposals accepted, will be reflected in the next version.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
view thread (416+ messages) latest in thread
Message-ID: <73630.1768923550@localhost>
Permalink: ../73630.1768923550@localhost/
Also on: postgresql.org/message-id/73630.1768923550@localhost
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: pgsql-hackers@postgresql.org
Cc: ah@cybertec.at, mihailnikalayeu@gmail.com, alvherre@alvh.no-ip.org, pgsql-hackers@lists.postgresql.org, rob@xzilla.net
Subject: Re: Adding REPACK [concurrently]
In-Reply-To: <73630.1768923550@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