Thread: Changed behavior in rewriteheap

On Thu, 21 Nov 2024, 17:18 Erik Nordström, <erik@timescale.com> 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)

On 22/11/2024 15:02, Matthias van de Meent wrote:
> I think I'd go with a patch like attached, where the bulk writer
> registers that it started with .relsize pages in the relfork, and use
> that for smgrextend() decisions. It now also records pages_written as
> a separate but accurate value.

Looks good to me. Eric, can you confirm that Matthias's patch fixes the
problem for you?

On 22/11/2024 15:56, Erik Nordström wrote:
> Yes, it solves the issue so it looks good.
>
> Just a minor nit: the code uses both blokno as local variable for
> pending_writes[i].blkno and directly accessing pending_writes[i].blkno.
> Maybe it is better to just use the local variable. For example, change
>
> ++ b/src/backend/storage/smgr/bulk_write.c
> @@ -304,7 +304,8 @@ smgr_bulk_flush(BulkWriteState *bulkstate)
>                          }
>
>                          smgrextend(bulkstate->smgr, bulkstate->forknum,
> blkno, page, true);
> -                       bulkstate->relsize = pending_writes[i].blkno + 1;
> +                       bulkstate->relsize++;
> +                       Assert(bulkstate->relsize == blkno + 1);
>
> Just a suggestion.

Made that change and committed to master and REL_17_STABLE. I didn't
bother with the assertion though. Also I removed the 'pages_written'
field, it was not used for anything anymore.