Rework compression activity wal markers #6920

JamesGuthrie · 2024-05-15T11:30:35Z

Adds WAL markers around all compression and decompression activities.
Renames the GUC controlling this behaviour.
Enables the WAL marker GUC by default.

This allows to distinguish between "user-driven" and
"compression-driven" DML on uncompressed chunks. This is a requirement
to be able to support DML on compressed chunks in live migration.

Note: A previous commit 1 added wal markers before and after inserts
which were part of "transparent decompression". Transparent
decompression is triggered when an UPDATE or DELETE statement affects
compressed data, or an INSERT statment inserts into a range of
compressed data which has a unique or primary key constraint. In these
cases, the data is first moved from the compressed chunk to the
uncompressed chunk, and then the DML is applied.

This change extends the existing behaviour on two fronts:

It adds WAL markers for both chunk compression and decompression
events.
It extends the WAL markers for transparent decompression to include
not only INSERTs into the compressed chunk, but also to TimescaleDB
catalog operations which were part of the decompression.

Harkishen-Singh · 2024-05-16T10:58:09Z

src/guc.c

@@ -477,10 +477,9 @@ _guc_init(void)
 NULL);

 DefineCustomBoolVariable(MAKE_EXTOPTION("enable_decompression_logrep_markers"),


At the moment, decompression markers sound specific to decompression activity. With this PR, the markers will be available for recompression as well. So, I think we should generalize the GUC to be compression markers (representing both decompression and recompression).

Suggested change

DefineCustomBoolVariable(MAKE_EXTOPTION("enable_decompression_logrep_markers"),

DefineCustomBoolVariable(MAKE_EXTOPTION("enable_compression_logrep_markers"),

I had considered that, but it would break compatibility with applications which are already using this guc. The way I justified retaining the name is to imagine that there are brackets around the "de" in decompression, like this: enable_(de)compression_logrep_markers.

I renamed this to enable_compression_wal_markers.

noctarius · 2024-05-17T11:07:09Z

If I understand the PR correctly, your patch wouldn't send the catalog updates for the chunk tables itself, would it? That would actually break functionality on my tool, since I completely replicate the catalog in memory and need that information.

I'd probably define a second set of markers with slightly different prefix names. In this case everybody would be able to decide to either ignore them or act on them. Alternatively, we could use the message body, like adding the table names, but I think the second prefix is easier and more obvious.

Changing the GUC name is ok, I can let people know in the docs :)

JamesGuthrie · 2024-05-21T07:49:11Z

If I understand the PR correctly, your patch wouldn't send the catalog updates for the chunk tables itself, would it?

This PR doesn't change which catalog updates are emitted. One thing that has changed is where exactly the "end decompression marker" is in the WAL. It now surrounds both inserts into the uncompressed chunk, and the associated catalog changes. To visualise this:

Previously the WAL would look like this for a "transparent decompression" event (e.g. UPDATE to compressed data):

BEGIN
message: transactional: 1 prefix: ::timescaledb-decompression-start, sz: 0 content:
table _timescaledb_internal.compress_hyper_2_2_chunk: DELETE: (no-tuple-data)
table _timescaledb_internal._hyper_1_1_chunk: INSERT: "time"[timestamp with time zone]:'2023-06-30 17:00:00-07' device_id[bigint]:1 value[double precision]:1
table _timescaledb_internal._hyper_1_1_chunk: INSERT: "time"[timestamp with time zone]:'2023-06-30 18:00:00-07' device_id[bigint]:1 value[double precision]:1
message: transactional: 1 prefix: ::timescaledb-decompression-end, sz: 0 content:
table _timescaledb_catalog.chunk: UPDATE: id[integer]:1 hypertable_id[integer]:1 schema_name[name]:'_timescaledb_internal' table_name[name]:'_hyper_1_1_chunk' compressed_chunk_id[integer]:2 dropped[boolean]:false status[integer]:9 osm_chunk[boolean]:false
table _timescaledb_internal._hyper_1_1_chunk: UPDATE: old-key: "time"[timestamp with time zone]:'2023-06-30 17:00:00-07' device_id[bigint]:1 value[double precision]:1 new-tuple: "time"[timestamp with time zone]:'2023-06-30 17:00:00-07' device_id[bigint]:1 value[double precision]:22
COMMIT

Now, it looks like this:

BEGIN
message: transactional: 1 prefix: ::timescaledb-decompression-start, sz: 0 content:
table _timescaledb_internal.compress_hyper_2_2_chunk: DELETE: (no-tuple-data)
table _timescaledb_internal._hyper_1_1_chunk: INSERT: "time"[timestamp with time zone]:'2023-06-30 17:00:00-07' device_id[bigint]:1 value[double precision]:1
table _timescaledb_internal._hyper_1_1_chunk: INSERT: "time"[timestamp with time zone]:'2023-06-30 18:00:00-07' device_id[bigint]:1 value[double precision]:1
table _timescaledb_catalog.chunk: UPDATE: id[integer]:1 hypertable_id[integer]:1 schema_name[name]:'_timescaledb_internal' table_name[name]:'_hyper_1_1_chunk' compressed_chunk_id[integer]:2 dropped[boolean]:false status[integer]:9 osm_chunk[boolean]:false
message: transactional: 1 prefix: ::timescaledb-decompression-end, sz: 0 content:
table _timescaledb_internal._hyper_1_1_chunk: UPDATE: old-key: "time"[timestamp with time zone]:'2023-06-30 17:00:00-07' device_id[bigint]:1 value[double precision]:1 new-tuple: "time"[timestamp with time zone]:'2023-06-30 17:00:00-07' device_id[bigint]:1 value[double precision]:22
COMMIT

Note that the timescaledb-decompression-end message now comes after the UPDATE to the _timescaledb_catalog.chunk table.

IMHO this is "more correct", as the decompression markers now surround all decompression activities. Your tool can decide which kinds of activities it does or does not ignore in the context of those markers.

noctarius · 2024-05-21T08:27:00Z

Ah I see. Yeah that is totally fine, since I can still act on all catalog entries, while ignoring anything outside the catalog 👍

antekresic

Great stuff, makes the replication markers more consistent across the whole codebase.

svenklemm · 2024-05-23T14:25:19Z

tsl/src/compression/api.c

@@ -728,6 +734,7 @@ tsl_compress_chunk_wrapper(Chunk *chunk, bool if_not_compressed, bool recompress
 ereport((if_not_compressed ? NOTICE : ERROR),
 (errcode(ERRCODE_DUPLICATE_OBJECT),
 errmsg("chunk \"%s\" is already compressed", get_rel_name(chunk->table_id))));
+ write_logical_replication_msg_compression_end();


This will not be triggered in the error path. I guess it doesnt matter if you want it in the error path you should have it above the ereport.

svenklemm

LGTM

tsl/src/compression/wal_utils.h

fabriziomello · 2024-05-27T20:26:05Z

tsl/src/compression/wal_utils.h

+#include "guc.h"
+#include <replication/message.h>


Suggested change

#include "guc.h"

#include <replication/message.h>

#pragma once

#include <postgres.h>

#include <replication/message.h>

#include "guc.h"

All headers should have the #pragma once to make sure it will be included only once. Also all headers should always include "postgres.h" as the first header.

- Adds WAL markers around all compression and decompression activities. - Renames the GUC controlling this behaviour. - Enables the WAL marker GUC by default. This allows to distinguish between "user-driven" and "compression-driven" DML on uncompressed chunks. This is a requirement to be able to support DML on compressed chunks in live migration. Note: A previous commit [1] added wal markers before and after inserts which were part of "transparent decompression". Transparent decompression is triggered when an UPDATE or DELETE statement affects compressed data, or an INSERT statment inserts into a range of compressed data which has a unique or primary key constraint. In these cases, the data is first moved from the compressed chunk to the uncompressed chunk, and then the DML is applied. This change extends the existing behaviour on two fronts: 1. It adds WAL markers for both chunk compression and decompression events. 2. It extends the WAL markers for transparent decompression to include not only INSERTs into the compressed chunk, but also to TimescaleDB catalog operations which were part of the decompression. [1]: b5b46a3

In timescale#6920 was introduced the dependency of Postgres contrib extension `test_decoding` for TAP tests but we forgot to include it in the sanitizer tests.

In #6920 was introduced the dependency of Postgres contrib extension `test_decoding` for TAP tests but we forgot to include it in the sanitizer tests.

JamesGuthrie force-pushed the jg/fun-with-compression branch 9 times, most recently from efd6c57 to 3f7717a Compare May 16, 2024 10:07

JamesGuthrie requested a review from svenklemm May 16, 2024 10:38

JamesGuthrie marked this pull request as ready for review May 16, 2024 10:38

Harkishen-Singh reviewed May 16, 2024

View reviewed changes

JamesGuthrie force-pushed the jg/fun-with-compression branch from 3f7717a to c972677 Compare May 21, 2024 07:49

antekresic approved these changes May 22, 2024

View reviewed changes

JamesGuthrie force-pushed the jg/fun-with-compression branch 4 times, most recently from 9e2dd49 to d5c0eed Compare May 22, 2024 14:27

svenklemm reviewed May 23, 2024

View reviewed changes

svenklemm approved these changes May 23, 2024

View reviewed changes

fabriziomello reviewed May 24, 2024

View reviewed changes

tsl/src/compression/wal_utils.h Show resolved Hide resolved

fabriziomello reviewed May 24, 2024

View reviewed changes

tsl/src/compression/wal_utils.h Outdated Show resolved Hide resolved

fabriziomello reviewed May 24, 2024

View reviewed changes

tsl/src/compression/wal_utils.h Outdated Show resolved Hide resolved

fabriziomello reviewed May 24, 2024

View reviewed changes

tsl/src/compression/wal_utils.h Outdated Show resolved Hide resolved

JamesGuthrie force-pushed the jg/fun-with-compression branch 2 times, most recently from 65aa711 to 51edab2 Compare May 27, 2024 11:27

JamesGuthrie changed the title ~~Full coverage of compression activity wal markers~~ Rework compression activity wal markers May 27, 2024

JamesGuthrie requested review from fabriziomello and antekresic May 27, 2024 11:28

antekresic approved these changes May 27, 2024

View reviewed changes

fabriziomello reviewed May 27, 2024

View reviewed changes

JamesGuthrie force-pushed the jg/fun-with-compression branch from 51edab2 to cbad494 Compare May 28, 2024 09:00

fabriziomello merged commit 3066605 into main May 28, 2024
35 checks passed

fabriziomello deleted the jg/fun-with-compression branch May 28, 2024 22:35

fabriziomello added a commit that referenced this pull request May 29, 2024

Fix sanitizer failure

8b2d0d2

In #6920 was introduced the dependency of Postgres contrib extension `test_decoding` for TAP tests but we forgot to include it in the sanitizer tests.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework compression activity wal markers #6920

Rework compression activity wal markers #6920

JamesGuthrie commented May 15, 2024 •

edited

Harkishen-Singh May 16, 2024

JamesGuthrie May 16, 2024

JamesGuthrie May 27, 2024

noctarius commented May 17, 2024

JamesGuthrie commented May 21, 2024 •

edited

noctarius commented May 21, 2024

antekresic left a comment

svenklemm May 23, 2024

svenklemm left a comment

fabriziomello May 27, 2024

JamesGuthrie May 28, 2024

		@@ -477,10 +477,9 @@ _guc_init(void)
		NULL);

		DefineCustomBoolVariable(MAKE_EXTOPTION("enable_decompression_logrep_markers"),

Rework compression activity wal markers #6920

Rework compression activity wal markers #6920

Conversation

JamesGuthrie commented May 15, 2024 • edited

Harkishen-Singh May 16, 2024

Choose a reason for hiding this comment

JamesGuthrie May 16, 2024

Choose a reason for hiding this comment

JamesGuthrie May 27, 2024

Choose a reason for hiding this comment

noctarius commented May 17, 2024

JamesGuthrie commented May 21, 2024 • edited

noctarius commented May 21, 2024

antekresic left a comment

Choose a reason for hiding this comment

svenklemm May 23, 2024

Choose a reason for hiding this comment

svenklemm left a comment

Choose a reason for hiding this comment

fabriziomello May 27, 2024

Choose a reason for hiding this comment

JamesGuthrie May 28, 2024

Choose a reason for hiding this comment

JamesGuthrie commented May 15, 2024 •

edited

JamesGuthrie commented May 21, 2024 •

edited