Update to Postgres 12 #1525

JLockerman · 2019-11-07T13:15:45Z

Currently all of the easy stuff (different function signatures and the like) is done, what remains is the more invasive changes, such as adding a compatibility layer to bridge the new TupleTableSlot-based and old tuple-based code.

erimatnor

Did a quick pass, but probably missed a bunch of stuff.

src/chunk_adaptive.c

src/bgw/job.c

src/chunk_adaptive.c

src/chunk_append/planner.c

src/chunk_append/exec.h

erimatnor · 2019-11-07T14:17:36Z

src/compat.h

+ * provide our own, temporary, implementation of index_get_next, which should be
+ * removed at some point when all our usages are replaced with index_getnext_slot
+ */
+// #if PG12


What's this commented code? Also C++ style comments.

These are TODOs, I'm not sure yet if the code is right. It's deliberately mis-styled as preventions from going in.

erimatnor · 2019-11-07T14:18:04Z

src/compat.h

@@ -578,6 +718,19 @@ get_attname_compat(Oid relid, AttrNumber attnum, bool missing_ok)
 #define PreventInTransactionBlock PreventTransactionChain
 #endif

+/*
+ * table_beginscan
+ * PG12 generalizes table scans to those no directly dependant on the heap.


Suggested change

* PG12 generalizes table scans to those no directly dependant on the heap.

* PG12 generalizes table scans to those not directly dependent on the heap.

Not sure I understand this formulation either.

erimatnor · 2019-11-07T14:19:19Z

src/compat.h

+/*
+ * table_beginscan
+ * PG12 generalizes table scans to those no directly dependant on the heap.
+ * These are the functions we should generally use for version after 12. For


Suggested change

* These are the functions we should generally use for version after 12. For

* These are the functions we should generally use for PG12 and later versions. For

erimatnor · 2019-11-07T14:27:26Z

src/tablespace.c

@@ -310,7 +310,7 @@ tablespace_insert(int32 hypertable_id, const char *tspcname)
 	Relation rel;
 	int32 id;

-	rel = heap_open(catalog_get_table_id(catalog, TABLESPACE), RowExclusiveLock);
+	rel = relation_open(catalog_get_table_id(catalog, TABLESPACE), RowExclusiveLock);
 	id = tablespace_insert_relation(rel, hypertable_id, tspcname);
 	heap_close(rel, RowExclusiveLock);


should this also change to relation_close?

erimatnor · 2019-11-07T14:29:20Z

src/bgw_policy/chunk_stats.c

@@ -114,7 +116,7 @@ ts_bgw_policy_chunk_stats_insert(BgwPolicyChunkStats *chunk_stats)
 {
 	Catalog *catalog = ts_catalog_get();
 	Relation rel =
-		heap_open(catalog_get_table_id(catalog, BGW_POLICY_CHUNK_STATS), RowExclusiveLock);
+		relation_open(catalog_get_table_id(catalog, BGW_POLICY_CHUNK_STATS), RowExclusiveLock);

 	ts_bgw_policy_chunk_stats_insert_with_relation(rel, chunk_stats);
 	heap_close(rel, RowExclusiveLock);


should this be relation_close? Otherwise mismatch with heap_open

@JLockerman I guess you will search for all presents of heap_close, which are many, right?

k-rus

Just few nits or questions. I am going to look more closely to the code.

k-rus · 2019-12-20T08:15:21Z

CMakeLists.txt

@@ -224,7 +223,7 @@ endif ()
 message(STATUS "Compiling against PostgreSQL version ${PG_VERSION}")

 if ((${PG_VERSION} VERSION_LESS "9.6")
-    OR (${PG_VERSION} VERSION_EQUAL "12")
+    # OR (${PG_VERSION} VERSION_EQUAL "12")


I guess the commented line is temporal and will be removed, right?

k-rus · 2019-12-20T08:20:44Z

src/bgw_policy/chunk_stats.c

@@ -114,7 +116,7 @@ ts_bgw_policy_chunk_stats_insert(BgwPolicyChunkStats *chunk_stats)
 {
 	Catalog *catalog = ts_catalog_get();
 	Relation rel =
-		heap_open(catalog_get_table_id(catalog, BGW_POLICY_CHUNK_STATS), RowExclusiveLock);
+		relation_open(catalog_get_table_id(catalog, BGW_POLICY_CHUNK_STATS), RowExclusiveLock);

 	ts_bgw_policy_chunk_stats_insert_with_relation(rel, chunk_stats);
 	heap_close(rel, RowExclusiveLock);


@JLockerman I guess you will search for all presents of heap_close, which are many, right?

k-rus · 2020-01-08T08:16:41Z

src/planner.c

+set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, Index rti, RangeTblEntry *rte);
+
+
+/* copied from allpaths.c */


Will it be good idea to move all copied code into a separate file? The rational is that the file is too big already and it will be easier to distinguish between the code origin.

k-rus

What was the reason to copy functions from allpaths.c? As I see no important modifications were done to them. What do I miss?

k-rus · 2020-01-08T10:16:32Z

src/planner.c

+			reenabled_inheritance = true;
+			//FIXME redo set_rel_consider_parallel
+			if (in_rel != NULL && in_rel->reloptkind == RELOPT_BASEREL)
+				set_rel_size(root, in_rel, i, in_rte);


This is a call to the modified function. Can it be done to the original one in PG after checking that children are of the correct kind?

Unfortunately not, IIRC the original function is marked static

k-rus · 2020-01-08T11:53:16Z

src/planner.c

+					set_plain_rel_size(root, rel, rte);
+				}
+				break;
+			case RTE_SUBQUERY:


Was this modification necessary? I see that the code between 11.4 and 12.1 is the same or very similar here.

It's not really necessary, it's just that these kinds should never appear as chunks. (the reason this function is copied in isn't because we need to make these modifications, they're just sanity-checking anyway, but because the original postgres function is marked static, and only called at a point where hypertables aren't expanded, providing the wrong info)

k-rus · 2020-01-10T09:23:44Z

@JLockerman Can you clarify why you don't use get_relation_info_hook to enable inheritance as it is done for PG11? Where is PG12 differ from PG11 in terms of calling get_relation_info, which calls the hook?

JLockerman · 2020-01-10T14:10:09Z

Before PG12,  planning was organized something like as follows:

 1. construct add RelOptInfo for base and appendrels
 2. add restrict info, joins, etc.
 3. perform the actual planning with make_one_rel

For our optimizations we would expand hypertables in the middle of
step 1; since nothing in the query planner before make_one_rel cared
about the inheritance children, we didn’t have to be too precise about
where we were doing it.

However, with PG12, and the optimizations around declarative
partitioning, postgres now does care about when the children are
expanded, since it wants as much information as possible to perform
partition-pruning. Now planning is organized like:

 1. construct add RelOptInfo for base rels only
 2. add restrict info, joins, etc.
 3. expand appendrels, removing irrelevant declarative partitions
 4. perform the actual planning with make_one_rel

Step 3 always expands appendrels, so when we also expand them during
step 1, the hypertable gets expanded twice, and things in the planner
break.

This commit attempts to solve this problem by keeping the hypertable
root marked as a non-inheritance table until make_one_rel is called,
and only then revealing to postgres that it does in fact have
inheritance children. While this Strategy entails the least code change
on our end, the fact that the first hook we can use to re-enable
inheritance is set_rel_pathlist_hook it does entail a number of
annoyances:

 1. this hook is called after the sizes of tables are calculated, so we
    must recalculate the sizes of all hypertables, as they will not
    have taken the chunk sizes into account
 2. the table upon which the hook is called will have its paths planned
    under the assumption it has no inheritance children, so if it's a
    hypertable we have to replan it's paths

Unfortunately, the code for doing these is static, so we need to copy
them into our own codebase, instead of just using postgres's.

k-rus

Few nits to this moment. I noticed that the code would not compile on PG11 (and fails locally), right?

k-rus · 2020-01-30T10:44:59Z

tsl/test/sql/continuous_aggs_query-11.sql

@@ -0,0 +1,189 @@
+-- This file and its contents are licensed under the Timescale License.


Is this file added on purpose? I don't see it in the CMake list.

nope, needs to be added to the .gitignore

k-rus · 2020-01-30T10:53:13Z

CMakeLists.txt

-    OR (${PG_VERSION} VERSION_EQUAL "12")
-    OR (${PG_VERSION} VERSION_GREATER "12"))
+    # OR (${PG_VERSION} VERSION_EQUAL "12")
+    OR (${PG_VERSION} VERSION_EQUAL "13"))
  message(FATAL_ERROR "TimescaleDB only supports PostgreSQL 9.6, 10 or 11")


Suggested change

message(FATAL_ERROR "TimescaleDB only supports PostgreSQL 9.6, 10 or 11")

message(FATAL_ERROR "TimescaleDB only supports PostgreSQL 9.6, 10, 11 or 12")

k-rus · 2020-01-30T10:58:08Z

src/compat.h

+ * any function that wishes to access these columns. In earlier versions, this
+ * parameter can be safely ignored.
+ */
+#if PG96 || PG10 || P11


Suggested change

#if PG96 || PG10 || P11

#if PG12_LT

Otherwise typo in P11.

k-rus · 2020-01-30T15:16:34Z

src/chunk_append/chunk_append.c

 			}

-			if (list_length(merge_childs) > 1)
+			if (merge_childs == NIL)


list_length(NIL) == 0, isn't it?

yeah, so we could write this as

if (list_length(merge_childs) < 1) { /*nop*/ } else if (list_length(merge_childs) == 1 ) { ... } else { ... }

I held off because it was a larger change. (Also I then to prefer the explicit NIL check to list_length == 0, though I'm not sure that'll hold in PG13)

Thanks. Now I see. It wasn't obvious by looking to PR in GH.
May be :

if (list_length(merge_childs) > 1) { ... } else if (list_length(merge_childs) == 1) { }

It will be less changes in the PR and more obvious to grasp. I guess further refactoring is for a separate issue.

k-rus · 2020-01-30T16:12:44Z

src/chunk_dispatch_state.c

 		}
-#if defined(USE_ASSERT_CHECKING) && PG11_GE
+#if defined(USE_ASSERT_CHECKING) && PG11
 		if (state->parent->mt_conflproj != NULL)
 		{
 			TupleTableSlot *slot = get_projection_info_slot_compat(


slot is already declared at the beginning of the function.

Since this seems to predate my changes, I'm going to hold off editting this unless it breaks PG11

k-rus · 2020-01-30T16:18:13Z

src/chunk_dispatch_state.c

@@ -143,7 +221,47 @@ chunk_dispatch_exec(CustomScanState *node)
 		MemoryContextSwitchTo(old);

 		/* Convert the tuple to the chunk's rowtype, if necessary */
-		tuple = ts_chunk_insert_state_convert_tuple(cis, tuple, &slot);
+#if PG12_LT
+		tuple = ts_chunk_insert_state_convert_tuple(cis, tuple, NULL);


Is it on purpose address to slot is not provided, so it is not set? Seems to be different to the original code.

I'll need to double check with PG11, I think I originally had the logic so that slot was always state->slot, so the store would be redundant, but that may have been on 12.

JLockerman · 2020-01-31T18:40:37Z

@k-rus ok, it works on PG11 now.

fix collation issues in nameeq, and our tests (+23 squashed commits) Squashed commits: [837314a] fix test ordering [67241b5] Fix INSERTs with dropped prefix columns columns [f95be0a] collation fixes part 3 [d56de21] better collation handling [24f4e86] insert_single passes [e2e48b9] collation handling for hash-partitions [3aacab9] copy fixes et.al. [02b47a0] Fix slot types [2d0588f] More WaitLatch fixes [b63f458] fix startup issues [719cd8c] index_getnext to index_getnext_slot conersion [ef11ca2] fix ExecFetch and ExecStore [7ad21ef] More oid and includes [0d0d8fe] process_utility changes [aa705c7] copy.c [5202855] use GetLockConflictsCompat [c2fd70e] plan_agg_bookend et.al. [ec770fd] more misc fixes [2c5886b] reorder.c refactor [d176200] use relation_open instead of heap_open, and other misc changes [018058c] We don't need to check if a table has oids anymore! [e3e45ff] Update function calls to new ABI [476fb6b] update to pg12

Before PG12, planning was organized something like as follows: 1. construct add RelOptInfo for base and appendrels 2. add restrict info, joins, etc. 3. perform the actual planning with make_one_rel For our optimizations we would expand hypertables in the middle of step 1; since nothing in the query planner before make_one_rel cared about the inheritance children, we didn’t have to be too precises about where we were doing it. However, with PG12, and the optimizations around declarative partitioning, postgres now does care about when the children are expanded, since it wants as much information as possible to perform partition-pruning. Now planning is organized like: 1. construct add RelOptInfo for base rels only 2. add restrict info, joins, etc. 3. expand appendrels, removing irrelevant declarative partitions 4. perform the actual planning with make_one_rel Step 3 always expands appendrels, so when we also expand them during step 1, the hypertable gets expanded twice, and things in the planner break. This commit attempts to solve this problem by keeping the hypertable root marked as a non-inheritance table until make_one_rel is called, and only then revealing to postgres that it does in fact have inheritance children. While this Strategy entails the least code change on our end, the fact that the first hook we can use to re-enable inheritance is set_rel_pathlist_hook it does entail a number of annoyances: 1. this hook is called after the sizes of tables are calculated, so we must recalculate the sizes of all hypertables, as they will not have taken the chunk sizes into account 2. the table upon which the hook is called will have its paths planned under the assumption it has no inheritance children, so if it's a hypertable we have to replan it's paths Unfortunately, the code for doing these is static, so we need to copy them into our own codebase, instead of just using postgres's. NOTE: the code for this commit is not yet complete, as restrictinfos aren't being used properly at the chunks.

…correct tts_ops (+1 squashed commit) Squashed commits: [9bbc54c] Fix more expansion issues, and segfault in chunk_adaptive

This required some careful handling of TupleDescs and TTSOps. Note that in chunk_dispatch_state.c we overwrite TupleTableSlot value, to ensure that we can swap the TupleDesc on it, despite the fact that such slots dynamically sized. This seems to be safe since the slot is always guaranteed to be at least basesize, and is only larger than that when it has a pinned TupleDesc; since we never want to have a pinned desc our TupleTableSlot should never be larger than the one previously residing in that memory. There may be a better solution for this.

Squashed commits: [c8c07b5] Fix issues uncovered by append test Fixes issues with ChunkAppend on multidimensional hypetables (postgres filters out some chunks), and TTSOps for ConstraintAwareAppend. Also adds all of the version specific regresstests [082be3f] Fix reloptions test WITH OIDS is no longer supported [d87c786] Fix reindex test Indexes cannot be opened as tables. [c8730a8] Fix pg_dump test output ordering [a9e1b58] Fix building on PG 12.1

PG12 treats first/last much like its own query, with it's own root. If we leave the hypertables marked as inheiritance parents, postgres will rexpand them, causing incorrect plans. We fix this by disabling inheritance again for all hypertables.

Assertions fire telling use we haven't locked the chunks correctly. Though it seems like we have (we lock them during expansion) re-lock them for now until we have a more principled fix.

…shed commit) Squashed commits: [0f22477] fix yet more collation issues

CommandCounterIncrement before the temporary relations are dropped.

We should still use the old hook to assign metadata for decompression, the new hook doesn't fire on the chunks. RTEs now have a field with the lock tables are to be opened with, we need to populate it.

allowed to modify, and is guaranteed to be VIRTUAL.

In older version, hypertables were expanded and metadata quals were manipulated in get_relation_info_hook. With pg12 we're forced to expand hypertables later, but the metadata can still be added in the original location. Unfortunately, we cannot remove unneeded functions (such as chunks_in) before hypertable expansion, so that must still be done there. We also need to remove such functions from the baserestrictinfo as it is now populated at this point. Eventually, we will likely wish to perform all of these optimizations on the baserestrictinfo, but this is left for future work.

The append_rel_list needs to set up in a very particular way for query planner to not crash, see inline comments for more details.

If removed from the jointree, such functions are not obeyed by the first/last optimization.

In PG12 heretofore unused fields of the aggregation info must be set for partitionwise aggregation to work correctly

Squashed commits: [5c58416] misc fixes and more test files

In PG12 UPDATE/DELETE on inheritance relations are planned in two stages: In stage 1, the statement is planned as if it was a SELECT and all leaf tables are discovered. In stage 2, the original query is planned against each leaf table, discovered in stage 1, directly, not part of an Append. Unfortunately, this means we cannot look in the appendrelinfo during UPDATE/DELETE plannin, in particular to determine if a table is a chunk, as the appendrelinfo is not at the point we wish to do so initialized. (see https://github.com/postgres/postgres/blob/REL_12_1/src/backend/optimizer/plan/planner.c#L1281-L1291 for more details on how planning is done now) This PR fixes this by switching our "are we performing DML on a chunk" code to look in the chunk catalog instead of the appendrelinfo. This is not the most efficient way to do this (in particular, we cannot not simply look up the chunk by oid, but must instead search by name), but other solutions, such as using our own hypertable expansion, failed with locking issues.

Squashed commits: [21442b4] Fix PR for PG11 Some code that only works was missing #ifdef's, and some changes only work on PG12. Of particular note are: gapfill: The strategy of copying into our own TupleTableSlot does not work as well in PG11, possibly because ExecCopyTuple copies a HeapTuple by default, instead of a virtual one. This may be salvageable with more effort. collations: In various places we add in collations to functions for PG12, as these are now required for TEXT hash and eq functions. We currently do not backport this behavior to PG11 in order to preserve the behavior users already have. chunk_append: PG11 has different behavior with single merge_children than PG12. This may have to do with Append-node removal in PG12. GetLockConflictsCompat: is moved to bgw/job.c so as to not require importing storage/lock.h in all files when job.c is the only place that uses it. [ca1d7c0] k-rus fixes [5ae3d4f] alternate_users should run on its own in 12 too [cb4e577] update some test results for PG12

erimatnor

So far this is only a partial review. But due to some major concerns, I figured I'd give this feedback sooner rather than later so we can discuss how to address.

The major concern I have is that this code doesn't properly adopt the new table access method API. The usage of the API throughout the code is inconsistent, and I believe the code is broken for anything else than standard heap storage.

Beyond this, I have the following list of concerns and nits:

Generally unclean code with lots of ifdefs, TODOs, commented code blocks, etc. I realize that there might be plans to address this, and some messy code paths we have to live with, but put together this gives me pause w.r.t. the state of this code.
No consistent use of new table AM API (doesn’t call, e.g., table_close after table_open)
Insert and copy code should be updated to deal with TupleTableSlots instead of heap tuples. Apart from being cleaner, it seems it is required to correctly insert into tables that do not use the heap storage method. In other words, doing, e.g., heap_insert instead of table_insert prevents routing the call to the correct access method for the table type. We need to provide versions of the table AM API functions for PG11 that maps this (only) to heap methods.
Unclear how support for generated columns are handled. Don’t see anything related to this in, e.g., COPY code and found no tests. (Might have missed, so correct me if wrong).
Unclear how WHERE clause support for COPY is handled (again, code seems to be simply missing. Maybe it is blocked?)

I have not yet touched on the rest of the code (there seems to be other major changes and code imports that needs to be reviewed).

Continuing my review in the meantime.

src/chunk_adaptive.c

erimatnor · 2020-01-30T11:00:48Z

src/chunk_adaptive.c


+#if PG12_LT


This looks really messy.

Seems it wouldn't be that hard to just implement a simple version of the new scan API for old PG versions (although I might be missing something). We'd just have to wrap the tuple in a tuple slot and provide some wrapper functions.

Alternatively, make the new API compatible with the old by unwrapping the slot to a tuple (which is already done in the code with ExecFetchSlotHeapTuple).

erimatnor · 2020-01-30T11:03:50Z

src/chunk_append/chunk_append.c

 			}

-			if (list_length(merge_childs) > 1)
+			if (merge_childs == NIL)


This looks odd. list_lenght() returns 0 for a NIL list so I don't see how this improves the previous code that only checked that the list was greater than 1.

erimatnor · 2020-02-04T08:51:18Z

src/bgw_policy/chunk_stats.c

@@ -114,7 +116,7 @@ ts_bgw_policy_chunk_stats_insert(BgwPolicyChunkStats *chunk_stats)
 {
 	Catalog *catalog = ts_catalog_get();
 	Relation rel =
-		heap_open(catalog_get_table_id(catalog, BGW_POLICY_CHUNK_STATS), RowExclusiveLock);
+		table_open(catalog_get_table_id(catalog, BGW_POLICY_CHUNK_STATS), RowExclusiveLock);

 	ts_bgw_policy_chunk_stats_insert_with_relation(rel, chunk_stats);
 	heap_close(rel, RowExclusiveLock);


Still a mismatch here. Should be table_close. Same thing in lots of other places.

src/bgw/job.c

src/chunk_dispatch_state.c

erimatnor · 2020-02-04T12:56:05Z

src/chunk_dispatch_state.c

+
+		Assert(state->slot->tts_tupleDescriptor == RelationGetDescr(cis->rel));
+		if (cis->tup_conv_map != NULL)
+			slot = execute_attr_map_slot(cis->tup_conv_map->attrMap, slot, state->slot);


execute_attr_map_slot seems to replace the old tuple conversion stuff, so I think we should realign with this API instead. In other words, this stuff should probably move into a function that hides the messy differences here.

And, why isn't ts_chunk_insert_state_convert_tuple called here for PG12, given that it is used for COPY and uses this function internally?

erimatnor · 2020-02-04T13:17:20Z

src/chunk_dispatch_state.c

 		tuple = ExecFetchSlotTuple(slot);
+#else /* TODO we should try not materialize a tuple if not needed */
+		tuple = ExecFetchSlotHeapTuple(slot, false, &should_free_tuple);


Why don't we just refactor this to just use a slot and then only get the relevant attributes required for tuple routing. In other words, ts_hyperspace_calculate_point should take a slot and just use slot_getattr

erimatnor · 2020-02-04T14:14:12Z

src/chunk_insert_state.c

@@ -561,7 +604,7 @@ ts_chunk_insert_state_create(Chunk *chunk, ChunkDispatch *dispatch)

 	/* Need a tuple table slot to store converted tuples */
 	if (state->tup_conv_map)
-		state->slot = MakeTupleTableSlotCompat(NULL);
+		state->slot = MakeTupleTableSlotCompat(NULL, ops);


Can't we get the ops from the chunk relation instead of having it passed in?

It seems to me that the slot ops here are specific to the chunk so we could just get the ops from table_slot_callbacks(rel)

erimatnor · 2020-02-05T11:31:45Z

src/copy.c

@@ -335,13 +368,28 @@ timescaledb_CopyFrom(CopyChunkState *ccstate, List *range_table, Hypertable *ht)

 				/* OK, store the tuple and create index entries for it */
 				heap_insert(resultRelInfo->ri_RelationDesc, tuple, mycid, hi_options, bistate);


I believe this needs to use table_tuple_insert or otherwise we won't invoke the right insert method for the table access method.

erimatnor · 2020-02-05T12:17:25Z

src/chunk_dispatch_state.c

+
+		cis = ts_chunk_dispatch_get_chunk_insert_state(dispatch, point, &cis_changed,
+#if PG12
+				state->slot->tts_ops


Can't we get the tts_ops from the chunk relation instead of passing it in?

all the misc cleanups will be getting squashed to here

k-rus · 2020-04-09T16:49:24Z

The final work, which started from this PR, is submitted as #1807

JLockerman requested a review from cevian November 7, 2019 13:15

erimatnor reviewed Nov 7, 2019

View reviewed changes

JLockerman force-pushed the pg12 branch from 12c20d4 to d176200 Compare November 7, 2019 15:21

k-rus self-requested a review December 19, 2019 14:42

k-rus reviewed Jan 8, 2020

View reviewed changes

k-rus self-requested a review January 8, 2020 08:19

k-rus reviewed Jan 8, 2020

View reviewed changes

erimatnor assigned JLockerman Jan 8, 2020

erimatnor added this to the 1.7.0 milestone Jan 8, 2020

JLockerman changed the title ~~[WIP] Update to Postgres 12~~ Update to Postgres 12 Jan 27, 2020

JLockerman requested a review from a team as a code owner January 28, 2020 19:32

JLockerman requested review from pmwkaa and erimatnor and removed request for a team January 28, 2020 19:32

RobAtticus mentioned this pull request Jan 29, 2020

timescaledb-tune fails when running on latest timscaledb helm chart (postgres 12) timescale/timescaledb-tune#61

Closed

JLockerman requested review from cevian, erimatnor and k-rus and removed request for cevian and erimatnor January 29, 2020 15:41

k-rus reviewed Jan 30, 2020

View reviewed changes

JLockerman requested a review from k-rus January 31, 2020 18:40

mfreed mentioned this pull request Feb 3, 2020

PostgreSQL 12 support #1456

Closed

JLockerman added 4 commits February 3, 2020 10:02

Prevent ChunkAppend from Assertion failing during query.sql due to in…

3508e5c

…correct tts_ops (+1 squashed commit) Squashed commits: [9bbc54c] Fix more expansion issues, and segfault in chunk_adaptive

JLockerman added 14 commits February 3, 2020 10:02

Fix first/last for PG12

ba374e0

PG12 treats first/last much like its own query, with it's own root. If we leave the hypertables marked as inheiritance parents, postgres will rexpand them, causing incorrect plans. We fix this by disabling inheritance again for all hypertables.

Fix parallel tests for PG12

cc2f614

Assertions fire telling use we haven't locked the chunks correctly. Though it seems like we have (we lock them during expansion) re-lock them for now until we have a more principled fix.

Don't try to open index as a table, and more collation fixes (+1 squa…

b9ce71c

…shed commit) Squashed commits: [0f22477] fix yet more collation issues

Fix "tuple already updated by self" in reorder by adding a

acacedc

CommandCounterIncrement before the temporary relations are dropped.

Fix decompression and locking issues therein

90b1b73

We should still use the old hook to assign metadata for decompression, the new hook doesn't fire on the chunks. RTEs now have a field with the lock tables are to be opened with, we need to populate it.

Fix gapfill for pg12 by storing subplan data in a slot we're actually

08b8fc5

allowed to modify, and is guaranteed to be VIRTUAL.

Fix first/last optimizations

8e24e3a

The append_rel_list needs to set up in a very particular way for query planner to not crash, see inline comments for more details.

Do not remove exclusion functions from the jointree in pg12

4f0467d

If removed from the jointree, such functions are not obeyed by the first/last optimization.

Fix partitionwise aggregation

3c3cd19

In PG12 heretofore unused fields of the aggregation info must be set for partitionwise aggregation to work correctly

Set RTE rellockmode for chunks (+1 squashed commit)

4711441

Squashed commits: [5c58416] misc fixes and more test files

JLockerman force-pushed the pg12 branch from 8f8ea40 to 81fffb5 Compare February 3, 2020 15:02

erimatnor requested changes Feb 5, 2020

View reviewed changes

erimatnor reviewed Feb 5, 2020

View reviewed changes

JLockerman force-pushed the pg12 branch from c205206 to 0e920e4 Compare February 5, 2020 15:24

cleanup commit

643316e

all the misc cleanups will be getting squashed to here

JLockerman force-pushed the pg12 branch from 0e920e4 to 643316e Compare February 5, 2020 16:03

k-rus mentioned this pull request Apr 9, 2020

Support PostgreSQL 12 #1807

Merged

k-rus closed this Apr 9, 2020

	* PG12 generalizes table scans to those no directly dependant on the heap.
	* PG12 generalizes table scans to those not directly dependent on the heap.

	* These are the functions we should generally use for version after 12. For
	* These are the functions we should generally use for PG12 and later versions. For

		set_rel_pathlist(PlannerInfo root, RelOptInfo rel, Index rti, RangeTblEntry *rte);


		/* copied from allpaths.c */

		@@ -0,0 +1,189 @@
		-- This file and its contents are licensed under the Timescale License.

	message(FATAL_ERROR "TimescaleDB only supports PostgreSQL 9.6, 10 or 11")
	message(FATAL_ERROR "TimescaleDB only supports PostgreSQL 9.6, 10, 11 or 12")

		@@ -335,13 +368,28 @@ timescaledb_CopyFrom(CopyChunkState ccstate, List range_table, Hypertable *ht)

		/* OK, store the tuple and create index entries for it */
		heap_insert(resultRelInfo->ri_RelationDesc, tuple, mycid, hi_options, bistate);

Update to Postgres 12 #1525

Update to Postgres 12 #1525

Conversation

JLockerman commented Nov 7, 2019

erimatnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-rus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-rus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLockerman Jan 8, 2020 • edited

Choose a reason for hiding this comment

k-rus commented Jan 10, 2020

JLockerman commented Jan 10, 2020

k-rus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLockerman Jan 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLockerman commented Jan 31, 2020

erimatnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-rus commented Apr 9, 2020

JLockerman Jan 8, 2020 •

edited

JLockerman Jan 30, 2020 •

edited