public 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: Tue, 20 Jan 2026 16:39:10 +0100
Message-ID: <73630.1768923550@localhost> (raw)
In-Reply-To: <CADzfLwUJSHKGxYw+vMUZ_Hr2YeuxO2Q5w13HKgUUN1725tjY5Q@mail.gmail.com>
References: <[email protected]>
<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 <[email protected]> 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 (31+ 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: <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