Subject: Re: Changed behavior in rewriteheap

From: Matthias van de Meent <[email-masked]>
Date: Fri, 22 Nov 2024 00:30:14 +0100

On Thu, 21 Nov 2024, 17:18 Erik Nordström, <[email-masked]> wrote:

> Hello,
>
> I've noticed a change in behavior of the heap rewrite functionality in
> PostgreSQL 17, used by, e.g., CLUSTER. I've been experimenting with the
> functionality to implement a way to merge partitions in TimescaleDB. I am
> using table_relation_copy_for_cluster() to write the data of several tables
> to a single merged table, and then I do a heap swap on one of the original
> tables while dropping the others. So, if you have 3 partitions and want to
> merge them to one, then I write all three partitions to a temporary heap,
> swap the new heap on partition 1 and then drop partitions 2 and 3.
>
> Now, this approach worked fine for PostgreSQL 15 and 16, but 17 introduced
> some changes that altered the behavior so that I only see data from one of
> the partitions after merge (the last one written).
>
> The commit that I think is responsible is the following:
> https://github.com/postgres/postgres/commit/8af256524893987a3e534c6578dd60edfb782a77
>
> Now, I realize that this is not a problem for PostgreSQL itself since the
> rewrite functionality isn't used for the purpose I am using it. To my
> defense, the rewrite code seems to imply that it should be possible to
> write more data to an existing heap according to this comment in
> begin_heap_rewrite: /* new_heap needn't be empty, just locked */.
>
> I've also tried recompiling PG17 with the rewriteheap.c file from PG16 and
> then it works again. I haven't yet been able to figure out exactly what is
> different but I will continue to try to narrow it down. In the meantime,
> maybe someone on the mailing list has some insight on what could be the
> issue and whether my approach is viable?
>

I think the origin of the issue is at bulk_write.c, in smgr_bulk_flush
(specifically after line 282), which assumes the bulk write system is used
exclusively on empty relations.

If you use a separate pair of begin/end_heap_rewrite for every relation you
merge into that heap, it'll re-initialize the bulk writer, which will thus
overwrite the previous rewrites' pages. The pre-PG17 rewriteheap.c doesn't
use that API, and thus isn't affected.

I've CC-ed Heikki as author of that patch; maybe a new API to indicate bulk
writes into an existing fork would make sense, to solve Timescale's issue
and fix rewriteheap's behavior?

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

---

From: Erik Nordström <[email-masked]>
Date: Fri, 22 Nov 2024 09:11:21 +0100

On Fri, Nov 22, 2024 at 12:30 AM Matthias van de Meent <
[email-masked]> wrote:

>> Now, this approach worked fine for PostgreSQL 15 and 16, but 17
>> introduced some changes that altered the behavior so that I only see data
>> from one of the partitions after merge (the last one written).
>>
>> I've also tried recompiling PG17 with the rewriteheap.c file from PG16
>> and then it works again. I haven't yet been able to figure out exactly what
>> is different but I will continue to try to narrow it down. In the meantime,
> If you use a separate pair of begin/end_heap_rewrite for every relation
> you merge into that heap, it'll re-initialize the bulk writer, which will
> thus overwrite the previous rewrites' pages. The pre-PG17 rewriteheap.c
> doesn't use that API, and thus isn't affected.
>
> I've CC-ed Heikki as author of that patch; maybe a new API to indicate
> bulk writes into an existing fork would make sense, to solve Timescale's
> issue and fix rewriteheap's behavior?
>
>
> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)
>

Yes, looks like it is that code that zeros out the initial pages if the
write doesn't start at block 0. And it looks like I don't have access to
the bulk write state to set pages_written to trick it to believe those
pages have already been written. Maybe it is possible to add an option to
the bulkwriter to skip zero-initialization? The comment for that code seems
to indicate it isn't strictly necessary anyway.

Regards,

Erik