Do not rely on cache on citus_drop_trigger #5372
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5372 +/- ##
=======================================
Coverage 92.76% 92.76%
=======================================
Files 215 215
Lines 45286 45332 +46
=======================================
+ Hits 42008 42052 +44
- Misses 3278 3280 +2 |
hanefi
force-pushed
the
fix-broken-drop-schema
branch
from
c79b2b1 to
176d998
Compare
| @@ -336,6 +337,16 @@ GetFunctionInfo(Oid typeId, Oid accessMethodId, int16 procedureId) | |||
| { | |||
| FmgrInfo *functionInfo = (FmgrInfo *) palloc0(sizeof(FmgrInfo)); | |||
|
|
|||
| /* if the type is being dropped, early exit with a null return value */ | |||
| HeapTuple tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typeId)); | |||
| if (!HeapTupleIsValid(tup)) | |||
There was a problem hiding this comment.
We should allow null heap tuples for a type only when we reach here via a drop trigger. There is no easy way to check if we are now doing a drop on the type.
| { | ||
| ereport(DEBUG4, (errmsg("could not get the catalog entries for type %u", | ||
| typeId))); | ||
| return functionInfo; |
There was a problem hiding this comment.
If the lookup for the type failed, we do not create function manager info, and hence we fail to sort the shard placements using the comparator functions defined for these types.
This may result in distributed deadlocks as we do not have a way to sort these placements and potentially we will try to drop the shards in an arbitrary order.
There was a problem hiding this comment.
can we return NULL here and have callers check for it? This 0 initialized struct seems a bit arbitrary.
hanefi
force-pushed
the
fix-broken-drop-schema
branch
from
176d998 to
3bd03b9
Compare
hanefi
force-pushed
the
fix-broken-drop-schema
branch
2 times, most recently
from
5f73f8e to
afae282
Compare
| NOTICE: drop cascades to 2 other objects | ||
| DETAIL: drop cascades to type comp_type | ||
| drop cascades to table range_dist_table_2 |
There was a problem hiding this comment.
idea: do we want to decrease verbosity to remove these lines?
hanefi
force-pushed
the
fix-broken-drop-schema
branch
from
1a87dd6 to
35501b9
Compare
| { | ||
| ereport(DEBUG4, (errmsg("could not get the catalog entries for type %u", | ||
| typeId))); | ||
| return functionInfo; |
There was a problem hiding this comment.
can we return NULL here and have callers check for it? This 0 initialized struct seems a bit arbitrary.
| char partitionMethod = PartitionMethodViaCatalog(relationId); | ||
| Var *partitionColumn = PartitionColumnViaCatalog(relationId); | ||
| GetIntervalTypeInfo(partitionMethod, partitionColumn, &intervalTypeId, | ||
| &intervalTypeMod); |
There was a problem hiding this comment.
how does this function behave when the type is gone?
There was a problem hiding this comment.
GetIntervalTypeInfo does not do any cache lookups. It only uses the values in the arguments partitionMethod, and partitionColumn to populate intervalTypeId and intervalTypeMod.
If the partitionColumn that we read from catalog contains intervalTypeId for an already dropped object, and the table is range distributed, we might end up with an invalid intervalTypeId value.
Do you think this is worth investigating more?
|
|
||
| char partitionMethod = PartitionMethodViaCatalog(relationId); | ||
| bool hashDistributed = partitionMethod == DISTRIBUTE_BY_HASH; | ||
| bool citusTableWithNoDistKey = partitionMethod == DISTRIBUTE_BY_NONE; |
There was a problem hiding this comment.
minor, maybe we can push the partitionMethod == DISTRIBUTE_BY_NONE logic into the internal function.
There was a problem hiding this comment.
I could not come up with an easy way to accomplish this.
Right now, ShouldSyncTableMetadata calls IsCitusTableTypeCacheEntry that uses the cache whereas ShouldSyncTableMetadataViaCatalog directly does the comparison.
Maybe I can use IsCitusTableTypeInternal(partitionMethod, replicationModel, tableType) directly. To be able to do that, I will need to lookup the replication model via catalog. That seems like too much work.
Another solution would be to introduce yet another helper function like IsCitusTableTypeInternal that does not depend on replicationModel params. This idea also defeats the main purpose of having similar codepaths in the functions that use the catalog or cache.
hanefi
force-pushed
the
fix-broken-drop-schema
branch
from
305714a to
4da4ab3
Compare
hanefi
force-pushed
the
fix-broken-drop-schema
branch
from
4da4ab3 to
c0d43d4
Compare
DESCRIPTION: Fixes a bug that prevents DROP SCHEMA CASCADE
Note: the current changes
arewere too intrusive and has a bad impact on performance. Iwill try to isolateisolated the calls that are made in thecitus_drop_triggercodepath to remove the impact on performance.TODO:
Fixes: #5099
Fixes: #3741