Use create_upper_paths_hook for hypertable insert plan modifications #904
Conversation
erimatnor
force-pushed
the
enordstr/refactor-insert-hooks-to-use-paths
branch
2 times, most recently
from
6b7a0e8
to
afd9ac2
Compare
Codecov Report
@@ Coverage Diff @@
## master #904 +/- ##
==========================================
- Coverage 91.25% 91.22% -0.04%
==========================================
Files 80 79 -1
Lines 9242 9227 -15
==========================================
- Hits 8434 8417 -17
- Misses 808 810 +2
Continue to review full report at Codecov.
|
erimatnor
force-pushed
the
enordstr/refactor-insert-hooks-to-use-paths
branch
2 times, most recently
from
dcb24d3
to
18b6c77
Compare
There was a problem hiding this comment.
Some minor comments. But I feel I should point out that now we are doing manipulation for each path where before we did it for each plan (and there are more paths than plans). This is probably the right thing to do anyway but thought I'd point out this potential downside.
erimatnor
force-pushed
the
enordstr/refactor-insert-hooks-to-use-paths
branch
5 times, most recently
from
d6e553f
to
fccce29
Compare
This change refactors how we do planner modifications to insert data into a hypertable. Instead of altering the finished plan tree produced by the standard planner, we now use the create_upper_paths_hook to do these modifications at the path stage during planning. Apart from being a cleaner, and arguably more principled, way of doing plan modifications, it also gives us better ability to modify plan state during planning since the hook happens during the normal planner run and not after. For instance, our CustomScan insert plan is now created using a callback that has access to the PlannerInfo data, which will be useful when planning, e.g., foreign queries using the foreign data wrapper API.
erimatnor
force-pushed
the
enordstr/refactor-insert-hooks-to-use-paths
branch
from
fccce29
to
995e864
Compare
| Index rti = lfirst_int(lc_rel); | ||
| RangeTblEntry *rte = planner_rt_fetch(rti, root); | ||
|
|
||
| ht = ts_hypertable_cache_get_entry(hcache, rte->relid); |
There was a problem hiding this comment.
I think it would be nice to add a check here that we are always talking about the same hypertable here... That we aren't somehow mixing hypertables.
|
Hi @erimatnor, I came across this PR while trying to find a solution for HA TimescaleDB(link). I have a HA postgres cluster setup using Patroni. I would like to have a HA TimescaleDB on top of it. I know that it requires streaming replication for this use. How do I make it highly available and fail safe? Basic solution that comes to my mind is running two Timescale DB instances and then using HA Proxy over it. What do you think about it? Is there any other way to do it? |
This change refactors how we do planner modifications to insert data
into a hypertable. Instead of altering the finished plan tree produced
by the standard planner, we now use the create_upper_paths_hook to do
these modifications at the path stage during planning.
Apart from being a cleaner, and arguably more principled, way of doing
plan modifications, it also gives us better ability to modify plan
state during planning since the hook happens during the normal planner
run and not after. For instance, our CustomScan insert plan is now
created using a callback that has access to the PlannerInfo data,
which will be useful when planning, e.g., foreign queries using the
foreign data wrapper API.
NOTE: this is a change that we need for clustering.