Re: Adding REPACK [concurrently]

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