PG-2470: Allow citus and timescaledb to compile#600
Conversation
jeltz
left a comment
jeltz
left a comment
There was a problem hiding this comment.
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.
dutow
force-pushed
the
pkg895b
branch
2 times, most recently
from
e5f570b to
f993f4f
Compare
Maybe we should add back the code formatting CI task to this repo? |
|
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? |
jeltz
left a comment
jeltz
left a comment
There was a problem hiding this comment.
I think we would need to also bump the percona_api_version, right?
Done! I also re-verified the exact behavior with citus/timescale: they compile, work, and error out when used together with pg_tde with the related features (create table using columnar or hypertable insert). The error seems to be safe to me. I also have an llm-generated tescript for testing them, we can add something based on that either to our normal CI or to our qa tests. |
dutow
changed the title
This commit reintroduces the old method signatures of some methods we changed to support pg_tde. The changed method signatures still exist with a "_percona" suffix, and all internal code calls the "_percona" methods. The reintroduced original methods also check a global flag which can disable them - if this happens, they report an 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. The check happens before any storage is created or WAL is written, so the failing statement is rolled back cleanly with no risk of data corruption. 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. This also bumps PERCONA_API_VERSION to 3, so that a pg_tde binary built against the previous API is refused by check_percona_api_version() instead of mismatching at the ABI level. Also verified that both citus and timescaledb do compile with these changes, and their basic test suite can be run againts our fork.
2 participants
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
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.