planner.avoidCachedDescriptor and mutable descriptors considered harmful

[Andrei Matei]

Yoz,

I want to groan about our use of table descriptor versions and their interactions with caching. As y'all know, our table descriptors are versioned and we have rules about what version someone operating at a given timestamp can use. One would think that a <version, descriptor> pair is immutable. But, alas, that's not the case: we mutate descriptors without incrementing their versions. For example, when dropping an index, we append a mutation to the descriptor and write it back without incrementing the version. That mutation matters - most code will consider the index to not exist when the mutation is present. So, depending on whether you use one descriptor or another you can get different results (e.g. for SHOW EXPERIMENTAL_FINGERPRINTS).
Now, we cache table descriptors, and we don't maintain multiple descriptors per version. So as soon as we add that mutation, the old version is gone from the cache. Look at what can happen because of that:
#25022
create table f(x int primary key, y int, index y_idx (y));
conn 1: begin
conn 2: drop index f@y_idx;
conn 1:
show experimental_fingerprints from table f; pq: index [3] not found

This is because our code tries to be aware of contexts in which it can use the cache and contexts in which it can't, but what you see above is an instance where it fails to do so: show experimental_fingerprints first avoids the cache and then uses the cache. There's planner.avoidCachedDescriptor which is used sometimes to... avoid the cache. However, I can't imagine a sane policy about where that should and shouldn't be set. Most uses of it are annotated with a TODO wondering if the setting is right.

There's also this old discussion about whether this fact that we're not always bumping descriptor versions can lead to data anomalies; at the very least it makes it very hard to reason about it:

https://beta.reviewable.io/reviews/cockroachdb/cockroach/16125#-KlUwyALTWq2r1mKKB_I


So in conclusion to the rant, I've got a question: do y'all think that we should move to immutable <version, descriptor> pairs? If we did, we could get rid of this avoidCachedDescriptor, right? Like, we should aim for all code to be oblivious of descriptor caching I think that'd be a pretty big improvement in this are of the code, which I think is one among the more in need of love areas as far as our code goes.

- a_m

[Vivek Menezes]

Andrei,

It's in our plan to complete https://github.com/cockroachdb/cockroach/issues/22275 which will substitute TableDescriptor with a new type extendedTableDescriptor. This will represent a descriptor at a particular version read at the version's ModififcationTime. Once that's done I believe TableDescriptor as a type should only be used by schema changes that are modifying the descriptor. Let's continue this discussion in that issue.

Thanks,

Vivek

--
You received this message because you are subscribed to the Google Groups "CockroachDB" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cockroach-db...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Andrei Matei]

#22275 seems to be about computed properties for a descriptor. I'm not sure how that speaks to the desire to make descriptors immutable (not write different bytes for the same table version)? In other words, "a descriptor at a particular version read at the version's ModififcationTime" is currently not unique.

To unsubscribe from this group and stop receiving emails from it, send an email to cockroach-db+unsubscribe@googlegroups.com.

[Raphael 'kena' Poss]

Hi Andrei,

thank you for raising this point.

In addition to Vivek's proposal to sanitize the API, I concur with you
that the addition of an index should also bump the descriptor version.
The fact this is not happening looks like an outright bug to me.

There are several things we can do:

- a check in the code that saves descriptors which verifies that no
version is written that already exists (i.e. no code should Put or CPut
a descriptor at version X if version X already exists).

- ensuring with Vivek's new API that descriptors handed out by the API
are not mutable, and that a separate API must be used to register a
mutation.


I see that you have created issue #25022 to highlight how the problem
surfaces in SHOW FINGERPRINTS. I think SHOW FINGERPRINTS can be altered
to avoid this bug, however that is a mere workaround.

Please also file a separate issue that tracks the
index-mutation-without-version-bump problem you have identified.

I think this would be a different issue from #22275, although I suppose
the person working on #22275 can work on both issues at once.

--
Raphael 'kena' Poss

[Vivek Menezes]

Ill..file.an issue to get rid of the upversion bit. That's where most of this stink comes from. Let me compose my thoughts into that issue.

[Andrei Matei]

+cc dan

>I see that you have created issue #25022 to highlight how the problem
> surfaces in SHOW FINGERPRINTS. I think SHOW FINGERPRINTS can be altered
> to avoid this bug, however that is a mere workaround.

The real issue I'm running into is that we have code that, in the middle of executing a statement, reuses the planner to plan an execute a different, "internal" statement. That's horrible, so I'm trying to migrate it to more clean execution of that internal statement (we generally should be avoiding the need for such internal statements, but whatevs). Except that, when reusing the planner, the inner statement was inheriting the avoidCachedDescriptor bit by design or by accident. In a cleaner execution, there's no such inheritance. So on occasions I have to do crap like look whether the outer statement had an AOST clause, and copy it in the inner statement. But that's quite horrible; everything about this avoidCachedDescriptor bothers me, because the timestamp at which a statement is running should be sufficient in deciding what descriptors it should use.

Now, there's also a related issue that comes up, that I generally don't know what to do about: the theology says that, at any timestamp, one can use either of two descriptor versions. They should produce equivalent results. Except we have code that behaves differently depending on the version - it's generally code that has special provisions for dealing with indexes or columns in the process of being added or dropped. EXPERIMENTAL_FINGERPRINTS is one such example - it fingerprints indexes in the process of being added. I suspect this was a bad idea, because now it suddenly means that if you use the other one of the two valid versions, one such index will not be seen (e.g. in an AOST statement you're more likely to not use the latest version valid for a timestamp, but the one before it). So based on the whim of the system, you might get one fingerprint or another at the same timestamp.
+Dan

[Daniel Harrison]

The "AllNonDropIndexes" call in EXPERIMENTAL_FINGERPRINTS is a bug. It should not be using indexes in mutations.

[Vivek Menezes]

Created issue https://github.com/cockroachdb/cockroach/issues/25030 . I'd ask that we continue this discussion on the issue and the other issue https://github.com/cockroachdb/cockroach/issues/25022

Thanks for bringing this up Andrei! Feel free to create more issues if appropriate.