Skip to content

PKG-895: Allow citus and timescaledb to compile#600

Open
dutow wants to merge 1 commit into
percona:PSP_REL_18_STABLEfrom
dutow:pkg895b
Open

PKG-895: Allow citus and timescaledb to compile#600
dutow wants to merge 1 commit into
percona:PSP_REL_18_STABLEfrom
dutow:pkg895b

Conversation

@dutow
Copy link
Copy Markdown
Collaborator

@dutow dutow commented May 11, 2026

Let's talk about this patch again!

Same as before, only for the PSP18 branch for now, 17 needs the same changes of course.

The pg_tde repo will need a matching

allow_upstream_smgr_api = false;

change if we go with this patch as-is.

This commit reintroduces the old method signatures of some methods we changed to support pg_tde. The changed method signatures still exists with a "2" suffix, and all internal code calls the "2" methods.

The reintroduced original methods also check a global flag which can disable them - if this happens, they report a FATAL error instead. This variable is activated by pg_tde, which means that if something tries to use these while pg_tde is loaded, and with that, possibly ignores its file tracking mechanism, it reports an error.

In practice this means that timescaledb, or the columnar storage in citus won't work if pg_tde is loaded in the shared_preload_libraries, but they can be used with our distribution without pg_tde enabled.

Also verified that both citus and timescaledb do compile with these changes, and their basic test suite can be run againts our fork.

@dutow dutow force-pushed the PSP_REL_18_STABLE branch from 50dd77c to a02a779 Compare May 12, 2026 10:20
Copy link
Copy Markdown
Collaborator

@jeltz jeltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apaart from some naming and style feedback I have one big question:

It feels a bit scary that we error out (now with FATAL but I think it could just be ERROR, not that that would help that much) not when Timescale or Citus are loaded but instead later when something uses them. But I can't really think of a better solution. We could of course have pg_tde warn if we see them loaded but we do not want to prevent them for using their own patched Citus so we can jsut warn which people probably will miss.

Maybe this is the least bad, hmm.

Comment thread src/backend/access/heap/heapam_handler.c Outdated
Comment thread src/backend/catalog/index.c Outdated
Comment thread src/backend/catalog/index.c Outdated
Comment thread src/backend/catalog/storage.c Outdated
Comment thread src/backend/storage/smgr/smgr.c Outdated
Comment thread src/backend/storage/smgr/smgr.c Outdated
This commit reintroduces the old method signatures of some methods we
changed to support pg_tde. The changed method signatures still exists
with a "2" suffix, and all internal code calls the "2" methods.

The reintroduced original methods also check a global flag which can
disable them - if this happens, they report a FATAL error instead. This
variable is activated by pg_tde, which means that if something tries to
use these while pg_tde is loaded, and with that, possibly ignores its
file tracking mechanism, it reports an error.

In practice this means that timescaledb, or the columnar storage in
citus won't work if pg_tde is loaded in the shared_preload_libraries,
but they can be used with our distribution without pg_tde enabled.

Also verified that both citus and timescaledb do compile with these
changes, and their basic test suite can be run againts our fork.
@dutow dutow requested a review from jeltz May 27, 2026 11:50
@dutow
Copy link
Copy Markdown
Collaborator Author

dutow commented May 27, 2026

This also does not follow PG's C style.

Maybe we should add back the code formatting CI task to this repo?

@jeltz
Copy link
Copy Markdown
Collaborator

jeltz commented May 27, 2026

I like the patch but maybe we should have a discussion on slack about my concern about ERRORing out in these code paths. Can it cause some bad harm to users or is it good enough?

Copy link
Copy Markdown
Collaborator

@jeltz jeltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would need to also bump the percona_api_version, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants