From da9af2c05d1be2b487c2d3f873a0de49bdb2740f Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Fri, 21 Oct 2022 22:39:38 +0400 Subject: [PATCH] Do not cache the classify_relation result It depends on the context, not only on the relation id. The same chunk can be expanded both as a child of hypertable and as an independent table. --- src/planner/expand_hypertable.c | 8 +- src/planner/planner.c | 209 ++++++++---------- src/planner/planner.h | 4 +- .../shared/expected/classify_relation.out | 11 + tsl/test/shared/sql/CMakeLists.txt | 1 + tsl/test/shared/sql/classify_relation.sql | 9 + 6 files changed, 123 insertions(+), 119 deletions(-) create mode 100644 tsl/test/shared/expected/classify_relation.out create mode 100644 tsl/test/shared/sql/classify_relation.sql diff --git a/src/planner/expand_hypertable.c b/src/planner/expand_hypertable.c index ed4fac81b..28d1c33c6 100644 --- a/src/planner/expand_hypertable.c +++ b/src/planner/expand_hypertable.c @@ -1394,13 +1394,9 @@ ts_plan_expand_hypertable_chunks(Hypertable *ht, PlannerInfo *root, RelOptInfo * /* * Add the information about chunks to the baserel info cache for - * classify_relation(). The relation type is TS_REL_CHUNK_CHILD -- chunk - * added as a result of expanding a hypertable. + * classify_relation(). */ - add_baserel_cache_entry_for_chunk(chunks[i]->table_id, - chunks[i]->fd.status, - ht, - TS_REL_CHUNK_CHILD); + add_baserel_cache_entry_for_chunk(chunks[i]->table_id, chunks[i]->fd.status, ht); } /* nothing to do here if we have no chunks and no data nodes */ diff --git a/src/planner/planner.c b/src/planner/planner.c index 83ab41233..3117f3686 100644 --- a/src/planner/planner.c +++ b/src/planner/planner.c @@ -75,8 +75,6 @@ typedef struct BaserelInfoEntry { Oid reloid; - /* Either a chunk or plain baserel (TS_REL_OTHER). */ - TsRelType type; Hypertable *ht; uint32 chunk_status; /* status of chunk, if this is a chunk */ @@ -157,11 +155,9 @@ static struct BaserelInfo_hash *ts_baserel_info = NULL; * chunk info at the plan time chunk exclusion. */ void -add_baserel_cache_entry_for_chunk(Oid chunk_reloid, uint32 chunk_status, Hypertable *hypertable, - TsRelType chunk_reltype) +add_baserel_cache_entry_for_chunk(Oid chunk_reloid, uint32 chunk_status, Hypertable *hypertable) { Assert(hypertable != NULL); - Assert(chunk_reltype == TS_REL_CHUNK || chunk_reltype == TS_REL_CHUNK_CHILD); if (ts_baserel_info == NULL) { @@ -173,14 +169,12 @@ add_baserel_cache_entry_for_chunk(Oid chunk_reloid, uint32 chunk_status, Hyperta if (found) { /* Already cached, check that the parameters are the same. */ - Assert(entry->type == chunk_reltype); Assert(entry->ht != NULL); Assert(entry->chunk_status == chunk_status); return; } /* Fill the cache entry. */ - entry->type = chunk_reltype; entry->ht = hypertable; entry->chunk_status = chunk_status; } @@ -651,21 +645,23 @@ get_parent_rte(const PlannerInfo *root, Index rti) return NULL; } -/* Fetch cached baserel entry. If it does not exists, create an entry for this - *relid. +/* + * Fetch cached baserel entry. If it does not exists, create an entry for this + * relid. * If this relid corresponds to a chunk, cache additional chunk * related metadata: like chunk_status and pointer to hypertable entry. * It is okay to cache a pointer to the hypertable, since this cache is * confined to the lifetime of the query and not used across queries. + * If the parent reolid is known, the caller can specify it to avoid the costly + * lookup. Otherwise pass InvalidOid. */ static BaserelInfoEntry * -get_or_add_baserel_from_cache(Oid chunk_relid, TsRelType chunk_reltype) +get_or_add_baserel_from_cache(Oid chunk_reloid, Oid parent_reloid) { Hypertable *ht = NULL; - TsRelType reltype = TS_REL_OTHER; /* First, check if this reloid is in cache. */ bool found = false; - BaserelInfoEntry *entry = BaserelInfo_insert(ts_baserel_info, chunk_relid, &found); + BaserelInfoEntry *entry = BaserelInfo_insert(ts_baserel_info, chunk_reloid, &found); if (found) { return entry; @@ -677,24 +673,27 @@ get_or_add_baserel_from_cache(Oid chunk_relid, TsRelType chunk_reltype) */ int32 hypertable_id = 0; int32 chunk_status = 0; - if (ts_chunk_get_hypertable_id_and_status_by_relid(chunk_relid, &hypertable_id, &chunk_status)) + if (ts_chunk_get_hypertable_id_and_status_by_relid(chunk_reloid, &hypertable_id, &chunk_status)) { /* * This is a chunk. Look up the hypertable for it. */ - reltype = chunk_reltype; // TS_REL_CHUNK or TS_REL_CHUNK_CHILD - Assert(chunk_reltype == TS_REL_CHUNK || chunk_reltype == TS_REL_CHUNK_CHILD); - Oid hypertable_relid = ts_hypertable_id_to_relid(hypertable_id); - ht = ts_planner_get_hypertable(hypertable_relid, CACHE_FLAG_NONE); + if (parent_reloid != InvalidOid) + { + /* Sanity check on the caller-specified hypertable reloid. */ + Assert(ts_hypertable_id_to_relid(hypertable_id) == parent_reloid); + } + else + { + /* Hypertable reloid not specified by the caller, look it up. */ + parent_reloid = ts_hypertable_id_to_relid(hypertable_id); + } + ht = ts_planner_get_hypertable(parent_reloid, CACHE_FLAG_NONE); Assert(ht != NULL); - } - else - { - Assert(reltype == TS_REL_OTHER); + Assert(ht->fd.id == hypertable_id); } /* Cache the result. */ - entry->type = reltype; entry->ht = ht; entry->chunk_status = chunk_status; return entry; @@ -707,97 +706,84 @@ get_or_add_baserel_from_cache(Oid chunk_relid, TsRelType chunk_reltype) * the first planner hook. */ static TsRelType -classify_relation(const PlannerInfo *root, const RelOptInfo *rel, Hypertable **p_ht) +classify_relation(const PlannerInfo *root, const RelOptInfo *rel, Hypertable **ht) { - RangeTblEntry *rte; - RangeTblEntry *parent_rte; - TsRelType reltype = TS_REL_OTHER; - Hypertable *ht = NULL; + Assert(ht != NULL); + *ht = NULL; - switch (rel->reloptkind) + if (rel->reloptkind != RELOPT_BASEREL && rel->reloptkind != RELOPT_OTHER_MEMBER_REL) { - case RELOPT_BASEREL: - rte = planner_rt_fetch(rel->relid, root); - /* - * To correctly classify relations in subqueries we cannot call - * ts_planner_get_hypertable with CACHE_FLAG_CHECK which includes - * CACHE_FLAG_NOCREATE flag because the rel might not be in cache yet. - */ - if (!OidIsValid(rte->relid)) - break; - ht = ts_planner_get_hypertable(rte->relid, CACHE_FLAG_MISSING_OK); - - if (ht != NULL) - reltype = TS_REL_HYPERTABLE; - else - { - /* - * This case is hit also by non-chunk BASERELs. We need a costly - * chunk metadata scan to distinguish between chunk and non-chunk - * baserel, so we cache the result of this lookup to avoid doing - * it repeatedly. - * - */ - BaserelInfoEntry *entry = get_or_add_baserel_from_cache(rte->relid, TS_REL_CHUNK); - ht = entry->ht; - reltype = entry->type; - } - break; - case RELOPT_OTHER_MEMBER_REL: - rte = planner_rt_fetch(rel->relid, root); - parent_rte = get_parent_rte(root, rel->relid); - - /* - * An entry of reloptkind RELOPT_OTHER_MEMBER_REL might still - * be a hypertable here if it was pulled up from a subquery - * as happens with UNION ALL for example. So we have to - * check for that to properly detect that pattern. - */ - if (parent_rte->rtekind == RTE_SUBQUERY) - { - ht = ts_planner_get_hypertable(rte->relid, - rte->inh ? CACHE_FLAG_MISSING_OK : CACHE_FLAG_CHECK); - - if (ht != NULL) - reltype = TS_REL_HYPERTABLE; - } - else - { - if (!OidIsValid(rte->relid)) - break; - ht = ts_planner_get_hypertable(parent_rte->relid, CACHE_FLAG_CHECK); - - if (ht != NULL) - { - if (parent_rte->relid == rte->relid) - reltype = TS_REL_HYPERTABLE_CHILD; - else - { - /* add cache entry for chunk child */ - BaserelInfoEntry *entry = - get_or_add_baserel_from_cache(rte->relid, TS_REL_CHUNK_CHILD); - if (entry->type != TS_REL_CHUNK_CHILD) - { - ereport(ERROR, - (errcode(ERRCODE_INTERNAL_ERROR), - errmsg("unexpected chunk type %d for chunk %s", - entry->type, - get_rel_name(entry->reloid)))); - } - reltype = TS_REL_CHUNK_CHILD; - } - } - } - break; - default: - Assert(reltype == TS_REL_OTHER); - break; + return TS_REL_OTHER; } - if (p_ht) - *p_ht = ht; + RangeTblEntry *rte = planner_rt_fetch(rel->relid, root); - return reltype; + if (!OidIsValid(rte->relid)) + { + return TS_REL_OTHER; + } + + if (rel->reloptkind == RELOPT_BASEREL) + { + /* + * To correctly classify relations in subqueries we cannot call + * ts_planner_get_hypertable with CACHE_FLAG_CHECK which includes + * CACHE_FLAG_NOCREATE flag because the rel might not be in cache yet. + */ + *ht = ts_planner_get_hypertable(rte->relid, CACHE_FLAG_MISSING_OK); + + if (*ht != NULL) + { + return TS_REL_HYPERTABLE; + } + + /* + * This is either a chunk seen as a standalone table, or a non-chunk + * baserel. We need a costly chunk metadata scan to distinguish between + * them, so we cache the result of this lookup to avoid doing it + * repeatedly. + */ + BaserelInfoEntry *entry = get_or_add_baserel_from_cache(rte->relid, InvalidOid); + *ht = entry->ht; + return *ht ? TS_REL_CHUNK_STANDALONE : TS_REL_OTHER; + } + + Assert(rel->reloptkind == RELOPT_OTHER_MEMBER_REL); + + RangeTblEntry *parent_rte = get_parent_rte(root, rel->relid); + + /* + * An entry of reloptkind RELOPT_OTHER_MEMBER_REL might still + * be a hypertable here if it was pulled up from a subquery + * as happens with UNION ALL for example. So we have to + * check for that to properly detect that pattern. + */ + if (parent_rte->rtekind == RTE_SUBQUERY) + { + *ht = ts_planner_get_hypertable(rte->relid, + rte->inh ? CACHE_FLAG_MISSING_OK : CACHE_FLAG_CHECK); + + return *ht ? TS_REL_HYPERTABLE : TS_REL_OTHER; + } + + if (parent_rte->relid == rte->relid) + { + /* + * A PostgreSQL table expansion peculiarity -- "self child", the root + * table that is expanded as a child of itself. This happens when our + * expansion code is turned off. + */ + *ht = ts_planner_get_hypertable(rte->relid, CACHE_FLAG_CHECK); + return *ht != NULL ? TS_REL_HYPERTABLE_CHILD : TS_REL_OTHER; + } + + /* + * Either an other baserel or a chunk seen when expanding the hypertable. + * Use the baserel cache to determine what it is. + */ + BaserelInfoEntry *entry = get_or_add_baserel_from_cache(rte->relid, parent_rte->relid); + *ht = entry->ht; + return *ht ? TS_REL_CHUNK_CHILD : TS_REL_OTHER; } extern void ts_sort_transform_optimization(PlannerInfo *root, RelOptInfo *rel); @@ -1056,7 +1042,7 @@ apply_optimizations(PlannerInfo *root, TsRelType reltype, RelOptInfo *rel, Range case TS_REL_HYPERTABLE_CHILD: /* empty table so nothing to optimize */ break; - case TS_REL_CHUNK: + case TS_REL_CHUNK_STANDALONE: case TS_REL_CHUNK_CHILD: ts_sort_transform_optimization(root, rel); break; @@ -1189,7 +1175,7 @@ timescaledb_set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, Index rti, Rang ts_planner_constraint_cleanup(root, rel); break; - case TS_REL_CHUNK: + case TS_REL_CHUNK_STANDALONE: case TS_REL_CHUNK_CHILD: if (IS_UPDL_CMD(root->parse)) @@ -1269,7 +1255,7 @@ timescaledb_get_relation_info_hook(PlannerInfo *root, Oid relation_objectid, boo ts_plan_expand_timebucket_annotate(root, rel); break; } - case TS_REL_CHUNK: + case TS_REL_CHUNK_STANDALONE: case TS_REL_CHUNK_CHILD: { ts_create_private_reloptinfo(rel); @@ -1355,7 +1341,8 @@ involves_hypertable(PlannerInfo *root, RelOptInfo *rel) if (rel->reloptkind == RELOPT_JOINREL) return join_involves_hypertable(root, rel); - return classify_relation(root, rel, NULL) == TS_REL_HYPERTABLE; + Hypertable *ht; + return classify_relation(root, rel, &ht) == TS_REL_HYPERTABLE; } /* diff --git a/src/planner/planner.h b/src/planner/planner.h index 24001b099..2180d6a56 100644 --- a/src/planner/planner.h +++ b/src/planner/planner.h @@ -75,7 +75,7 @@ ts_get_private_reloptinfo(RelOptInfo *rel) typedef enum TsRelType { TS_REL_HYPERTABLE, /* A hypertable with no parent */ - TS_REL_CHUNK, /* Chunk with no parent (i.e., it's part of the + TS_REL_CHUNK_STANDALONE, /* Chunk with no parent (i.e., it's part of the * plan as a standalone table. For example, * querying the chunk directly and not via the * parent hypertable). */ @@ -107,6 +107,6 @@ extern void ts_planner_constraint_cleanup(PlannerInfo *root, RelOptInfo *rel); extern Node *ts_add_space_constraints(PlannerInfo *root, List *rtable, Node *node); extern void add_baserel_cache_entry_for_chunk(Oid chunk_reloid, uint32 chunk_status, - Hypertable *hypertable, TsRelType chunk_reltype); + Hypertable *hypertable); #endif /* TIMESCALEDB_PLANNER_H */ diff --git a/tsl/test/shared/expected/classify_relation.out b/tsl/test/shared/expected/classify_relation.out new file mode 100644 index 000000000..4b5bcd1ae --- /dev/null +++ b/tsl/test/shared/expected/classify_relation.out @@ -0,0 +1,11 @@ +-- This file and its contents are licensed under the Timescale License. +-- Please see the included NOTICE for copyright information and +-- LICENSE-TIMESCALE for a copy of the license. +-- Test the case where the chunk is present both as a separate table and as a +-- child of a hypertable. #4708 +select show_chunks('metrics_compressed') chunk order by 1 limit 1 \gset +select * from metrics_compressed inner join :chunk on (false); + time | device_id | v0 | v1 | v2 | v3 | time | device_id | v0 | v1 | v2 | v3 +------+-----------+----+----+----+----+------+-----------+----+----+----+---- +(0 rows) + diff --git a/tsl/test/shared/sql/CMakeLists.txt b/tsl/test/shared/sql/CMakeLists.txt index 951a7dd16..011107087 100644 --- a/tsl/test/shared/sql/CMakeLists.txt +++ b/tsl/test/shared/sql/CMakeLists.txt @@ -1,5 +1,6 @@ set(TEST_FILES_SHARED cagg_compression.sql + classify_relation.sql constify_timestamptz_op_interval.sql constraint_exclusion_prepared.sql decompress_placeholdervar.sql diff --git a/tsl/test/shared/sql/classify_relation.sql b/tsl/test/shared/sql/classify_relation.sql new file mode 100644 index 000000000..20059da70 --- /dev/null +++ b/tsl/test/shared/sql/classify_relation.sql @@ -0,0 +1,9 @@ +-- This file and its contents are licensed under the Timescale License. +-- Please see the included NOTICE for copyright information and +-- LICENSE-TIMESCALE for a copy of the license. + +-- Test the case where the chunk is present both as a separate table and as a +-- child of a hypertable. #4708 + +select show_chunks('metrics_compressed') chunk order by 1 limit 1 \gset +select * from metrics_compressed inner join :chunk on (false);