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

@@ -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
+		 * classify_relation().
-		 * added as a result of expanding a hypertable.
 		 */
-		add_baserel_cache_entry_for_chunk(chunks[i]->table_id,
+		add_baserel_cache_entry_for_chunk(chunks[i]->table_id, chunks[i]->fd.status, ht);
-										  chunks[i]->fd.status,
-										  ht,
-										  TS_REL_CHUNK_CHILD);
 	}
 
 	/* nothing to do here if we have no chunks and no data nodes */

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,
+add_baserel_cache_entry_for_chunk(Oid chunk_reloid, uint32 chunk_status, Hypertable *hypertable)
-								  TsRelType chunk_reltype)
 {
 	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
+		if (parent_reloid != InvalidOid)
-		Assert(chunk_reltype == TS_REL_CHUNK || chunk_reltype == TS_REL_CHUNK_CHILD);
+		{
-		Oid hypertable_relid = ts_hypertable_id_to_relid(hypertable_id);
+			/* Sanity check on the caller-specified hypertable reloid. */
-		ht = ts_planner_get_hypertable(hypertable_relid, CACHE_FLAG_NONE);
+			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);
-	}
+		Assert(ht->fd.id == hypertable_id);
-	else
-	{
-		Assert(reltype == TS_REL_OTHER);
 	}
 
 	/* 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;
+	Assert(ht != NULL);
-	RangeTblEntry *parent_rte;
+	*ht = NULL;
-	TsRelType reltype = TS_REL_OTHER;
-	Hypertable *ht = NULL;
 
-	switch (rel->reloptkind)
+	if (rel->reloptkind != RELOPT_BASEREL && rel->reloptkind != RELOPT_OTHER_MEMBER_REL)
 	{
-		case RELOPT_BASEREL:
+		return TS_REL_OTHER;
-			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;
 	}
 
-	if (p_ht)
+	RangeTblEntry *rte = planner_rt_fetch(rel->relid, root);
-		*p_ht = ht;
 
-	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;
 }
 
 /*

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 */

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)
+

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

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);