Commit 634b2d4
scylla,conn,frame: negotiate SCYLLA_USE_METADATA_ID and skip result metadata under it
Prepared-statement result metadata could not be invalidated safely: after an
ALTER the server kept answering with the old column set, so a driver reusing the
metadata it cached at prepare time decoded rows against columns that no longer
described the response (scylladb/scylladb#20860). gocql's answer was to stop
reusing it — f292aaf ("Disable skipping metadata by default") flipped
DisableSkipMetadata to default true — at the cost of carrying result metadata on
every response.
scylladb/scylladb#23292 fixes the underlying problem for SELECT statements. A
server advertising SCYLLA_USE_METADATA_ID hands out a result metadata ID at
prepare time; the driver returns that ID with every EXECUTE, and a stale ID is
answered with the METADATA_CHANGED flag plus fresh metadata and a fresh ID. That
is native protocol v5's mechanism made available on v4, which is what Scylla
negotiates. Implement the driver half.
Negotiation and plumbing.
scyllaUseMetadataIDExt implements cqlProtocolExtension and is registered in
parseCQLProtocolExtensions, so it is sent in STARTUP whenever SUPPORTED lists the
key. The v5 metadata-ID gates in frame.go widen from `proto > protoVersion4` to
`proto > protoVersion4 || scyllaUseMetadataID`: the read in parseResultPrepared,
the read in parseResultMetadata behind METADATA_CHANGED, and the write in
writeExecuteFrame. A v4 connection that negotiated the extension therefore drives
the same primitives the v5 port already built, rather than a second
implementation of them.
Detection is consolidated onto the extension. parseSupported already keyed
isMetadataIDSupported — and through it the isScylla heuristic and the
IsMetadataIDSupported() getter — off a function-local SCYLLA_USE_METADATA_ID
const. That const moves to package scope and both the detector and the extension
read it, leaving one spelling and one detector for the capability.
One source of truth for the negotiated flag.
Two independently derived booleans governing the two halves of one wire contract
is a bug waiting to happen: a Conn that believed the extension was on while its
framers did not would ask the server to skip result metadata while writing no ID
for it to compare against, and the driver would then decode rows against whatever
metadata it had cached. So the negotiated state lives in framerConfig, populated
by connFramers.initCache during connection setup before any query can run, and
Conn reads it through the usesMetadataID() and tracksResultMetadataID()
accessors. There is no Conn-level copy to diverge from it.
newFramerWithExts derived the same flags a second time and is deleted. It had no
non-test callers — production framers come from the per-Conn pool — so keeping it
meant every future extension had to be handled in two places, with only a comment
to say so. Its call sites in scylla_test.go now go through initFramerCache and
getWriteFramer, i.e. the path production uses, which is what makes the claim above
true rather than aspirational.
One framer is still not built from framerConfig: framerPool.get falls back to
newFramer when the pool is disabled, which yields scyllaUseMetadataID false whatever
was negotiated. That is correct during the handshake, and unreachable from the
request path afterwards, because execInternal takes its framer before addCall
rejects a closed connection — so a framer taken after the pool closed belongs to a
call that never writes a frame. The accessor's doc comment says so rather than
claiming an invariant that holds by call ordering rather than by construction; scylladb#982
tracks the fix, which also covers flagLWT and tabletsRoutingV1.
Skipping result metadata.
shouldSkipResultMetadata replaces the inline skipMeta expression in
executeQueryWithMetrics, and metadataIDTracked gates it on both halves of the
mechanism: the connection exchanges result metadata IDs *and* the prepared
statement carries a non-empty one. Where both hold, the session-level
DisableSkipMetadata is ignored, including when it was set to true explicitly — the
flag is a workaround for the bug this mechanism fixes, so once the server reports
metadata changes there is nothing left to work around. Upstream gocql skips by
default on every protocol version, and there is deliberately no session-level knob
to force metadata back on; the java-driver's
skip-cql4-metadata-resolve-method has no equivalent here.
The ID exchange is active on native protocol v5, where the field is mandatory, as
well as on v4 with the extension, and Conn.tracksResultMetadataID reports either.
Scoping the override to the extension alone would leave gocql with opposite
defaults for two encodings of one mechanism, and the losing one would be the one
where the ID is guaranteed by the protocol rather than negotiated: a v5 connection
would carry full result metadata on every response for no reason.
scylladb/scylla-drivers#81 states the rule as "if SCYLLA_USE_METADATA_ID was
negotiated or CQL v5 is used", and the java-driver reaches it from the other
direction — DefaultPreparedStatement.resolveSkipMetadata returns true for any
non-empty result metadata ID, which v5 always supplies. The python-driver
implements the extension half only.
The second condition, a non-empty ID, is reachable and matters. The prepared cache
is keyed (hostID, keyspace, statement) and is evicted only on prepare failure or
UNPREPARED, never on connection close, so a statement prepared before the
extension was negotiated survives a reconnect onto a now-extension-enabled
connection. Without the gate the driver would request skip_metadata while sending
an empty ID. With it, such a statement asks for metadata for one more round trip,
acquires an ID from the resulting METADATA_CHANGED response, and skips from then
on — leaving no window in which the driver skips metadata it cannot recover. Both
sibling drivers gate on the same condition (scylladb/python-driver#770,
scylladb/java-driver#599 and follow-ups).
The remaining gate, a non-empty cached column set, is not an optimization either.
A statement whose RESULT/Prepared carries no result metadata is handed an ID
hashed from empty metadata; current Scylla compares the returned ID against that
same empty-metadata ID, always matches, and so never sets METADATA_CHANGED,
leaving a driver that asked to skip with a response it has no columns to decode.
LIST ROLES OF is the motivating case. The server-side fixes,
scylladb/scylladb#29233 and scylladb/scylladb#29275, are both closed unmerged, so
this gate is what keeps such statements working; document and test it as such.
Query.NoSkipMetadata wins in every case, and is now the only way to force metadata
where the ID exchange is active. Conditional statements are the case to keep in
mind, since their response column set depends on whether the condition applied —
something a result metadata ID cannot express, as it describes the statement and
not the outcome. In practice the column-set gate already covers them, because a
prepared conditional statement's result metadata is empty, and ScanCAS and
MapScanCAS set NoSkipMetadata internally regardless.
A RESULT/Rows that sets METADATA_CHANGED while also setting NO_METADATA is
rejected. The combination is malformed — METADATA_CHANGED obliges the server to
include the new metadata — and both ways of continuing are unrecoverable. Adopting
the ID while keeping the old columns lets the server match it from then on and stop
sending metadata, leaving the driver decoding against stale columns indefinitely;
the python-driver guards that identically. But returning the response's own rows is
the same misdecode one execute earlier: the server has just declared those columns
stale, and the skip-metadata path below would decode against them anyway. So do
neither, and fail the query with the old ID left in the cache — a retry resends it
and the server gets another chance to answer with the metadata it owes.
Record and replay.
The record/replay dialers hash EXECUTE frames at fixed offsets, and skipped the
resultMetadataID field only for protocol v5+. Under this extension that field also
appears on v4 EXECUTE frames, which the frame bytes alone cannot reveal, so the
negotiated state is plumbed through instead: StartupNegotiatesMetadataID detects
the opt-in on both the record and the replay path, the recorder latches it and
stamps each Record with UseMetadataID, and GetFrameHash takes it as an argument.
Documentation.
The DisableSkipMetadata comment said the driver "may still" send skip_metadata
under the extension, which understates it: the flag defaults to true, so the
override is the normal case rather than an exception, and "Default: true" is
misleading on its own. It now says plainly that the flag is ignored — explicit
values included — which connections that applies to, and why that is safe. The
Validate() warning fires on !DisableSkipMetadata, so after the override the
population that actually gets skipping is unwarned while those who opted in
explicitly still are. The protocol version is negotiated per connection, and the
extension long after Validate runs, so the trigger cannot be narrowed; the message
instead names the case that is still risky — a connection that exchanges no result
metadata ID at all.
Tests.
Unit coverage for the extension's negotiation, registration and serialization
(scylla_test.go); for initFramerCache and usesMetadataID against the framer
config, with a negative counterpart for the not-negotiated case; for
tracksResultMetadataID over both mechanisms and both protocol versions, including
that it masks the request/response direction bit, and that usesMetadataID stays
narrower so a v5 connection cannot pass for a negotiated extension; for
shouldSkipResultMetadata composed with metadataIDTracked the way the EXECUTE path
composes them, over the nil and zero-length ID cases, an ID without an ID
exchange, an explicit opt-in, and the empty-column-set gate; and for the
v4-plus-extension GetFrameHash skip. A regression test pins that a truncated
resultMetadataID in a RESULT/Prepared frame is reported as an error through
parseFrame's recover rather than panicking the serve goroutine — the extension
makes that short-bytes read live on protocol v4.
TestPrepareExecuteMetadataChangedFlag becomes table-driven over both ways the ID
exchange can be active, rather than growing a second near-verbatim copy of its
~150-line flow for the extension. It also drops and recreates its table instead of
CREATE IF NOT EXISTS, because the flow ALTERs that table and one left over from an
earlier run already has the added column; and it asserts the no-change case by
pointer identity on the cache entry, since comparing the entry's fields compares it
with itself and can never fail. The extension case stops skipping unconditionally:
it skips only when the server does not advertise the capability, and fails when the
server advertises it but negotiation did not happen, since it is the only
end-to-end coverage of the feature and a negotiation regression must not be able to
turn it green. The v5 case does not run at the suite's default protocol, since
TEST_CQL_PROTOCOL is pinned to 4; the tests commit later in this series gives it an
explicit run against Cassandra 5.
One occurrence of the Id spelling is deliberate, in frame_test.go's transcription
of Cassandra's ResultSet$ResultMetadata$Codec.encode — a verbatim quote of Java
source, which keeps its original naming.
Fixes: https://scylladb.atlassian.net/browse/DRIVER-152
Fixes: scylladb#527
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>1 parent 4bee517 commit 634b2d4
15 files changed
Lines changed: 928 additions & 189 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3657 | 3657 | | |
3658 | 3658 | | |
3659 | 3659 | | |
3660 | | - | |
3661 | | - | |
| 3660 | + | |
3662 | 3661 | | |
3663 | | - | |
| 3662 | + | |
3664 | 3663 | | |
3665 | 3664 | | |
3666 | 3665 | | |
3667 | | - | |
3668 | | - | |
| 3666 | + | |
| 3667 | + | |
| 3668 | + | |
3669 | 3669 | | |
3670 | 3670 | | |
3671 | | - | |
| 3671 | + | |
| 3672 | + | |
| 3673 | + | |
| 3674 | + | |
| 3675 | + | |
| 3676 | + | |
| 3677 | + | |
| 3678 | + | |
| 3679 | + | |
| 3680 | + | |
| 3681 | + | |
| 3682 | + | |
| 3683 | + | |
3672 | 3684 | | |
3673 | | - | |
3674 | | - | |
| 3685 | + | |
| 3686 | + | |
| 3687 | + | |
| 3688 | + | |
| 3689 | + | |
| 3690 | + | |
| 3691 | + | |
| 3692 | + | |
| 3693 | + | |
| 3694 | + | |
| 3695 | + | |
| 3696 | + | |
| 3697 | + | |
| 3698 | + | |
| 3699 | + | |
| 3700 | + | |
| 3701 | + | |
| 3702 | + | |
| 3703 | + | |
| 3704 | + | |
| 3705 | + | |
| 3706 | + | |
| 3707 | + | |
| 3708 | + | |
| 3709 | + | |
| 3710 | + | |
| 3711 | + | |
| 3712 | + | |
| 3713 | + | |
| 3714 | + | |
| 3715 | + | |
| 3716 | + | |
| 3717 | + | |
| 3718 | + | |
| 3719 | + | |
| 3720 | + | |
| 3721 | + | |
| 3722 | + | |
| 3723 | + | |
| 3724 | + | |
| 3725 | + | |
| 3726 | + | |
3675 | 3727 | | |
3676 | | - | |
3677 | | - | |
| 3728 | + | |
| 3729 | + | |
| 3730 | + | |
| 3731 | + | |
| 3732 | + | |
| 3733 | + | |
| 3734 | + | |
| 3735 | + | |
| 3736 | + | |
| 3737 | + | |
| 3738 | + | |
| 3739 | + | |
3678 | 3740 | | |
| 3741 | + | |
| 3742 | + | |
| 3743 | + | |
| 3744 | + | |
| 3745 | + | |
| 3746 | + | |
| 3747 | + | |
| 3748 | + | |
| 3749 | + | |
3679 | 3750 | | |
3680 | | - | |
| 3751 | + | |
| 3752 | + | |
| 3753 | + | |
| 3754 | + | |
| 3755 | + | |
| 3756 | + | |
| 3757 | + | |
3681 | 3758 | | |
3682 | 3759 | | |
3683 | 3760 | | |
| 3761 | + | |
| 3762 | + | |
| 3763 | + | |
| 3764 | + | |
| 3765 | + | |
| 3766 | + | |
| 3767 | + | |
| 3768 | + | |
| 3769 | + | |
| 3770 | + | |
| 3771 | + | |
| 3772 | + | |
3684 | 3773 | | |
3685 | 3774 | | |
3686 | 3775 | | |
| |||
3689 | 3778 | | |
3690 | 3779 | | |
3691 | 3780 | | |
3692 | | - | |
3693 | | - | |
| 3781 | + | |
3694 | 3782 | | |
3695 | 3783 | | |
3696 | 3784 | | |
3697 | | - | |
3698 | | - | |
3699 | | - | |
3700 | | - | |
3701 | | - | |
3702 | | - | |
3703 | | - | |
| 3785 | + | |
| 3786 | + | |
3704 | 3787 | | |
3705 | | - | |
3706 | | - | |
| 3788 | + | |
3707 | 3789 | | |
3708 | 3790 | | |
3709 | 3791 | | |
| |||
3714 | 3796 | | |
3715 | 3797 | | |
3716 | 3798 | | |
3717 | | - | |
3718 | | - | |
3719 | | - | |
3720 | | - | |
3721 | | - | |
| 3799 | + | |
| 3800 | + | |
3722 | 3801 | | |
3723 | 3802 | | |
3724 | 3803 | | |
3725 | 3804 | | |
3726 | 3805 | | |
3727 | 3806 | | |
3728 | 3807 | | |
3729 | | - | |
3730 | | - | |
3731 | | - | |
| 3808 | + | |
3732 | 3809 | | |
3733 | 3810 | | |
3734 | 3811 | | |
3735 | | - | |
3736 | | - | |
| 3812 | + | |
3737 | 3813 | | |
3738 | 3814 | | |
3739 | 3815 | | |
3740 | 3816 | | |
3741 | | - | |
| 3817 | + | |
3742 | 3818 | | |
3743 | | - | |
3744 | | - | |
| 3819 | + | |
| 3820 | + | |
3745 | 3821 | | |
3746 | 3822 | | |
3747 | 3823 | | |
| |||
3759 | 3835 | | |
3760 | 3836 | | |
3761 | 3837 | | |
3762 | | - | |
| 3838 | + | |
3763 | 3839 | | |
3764 | 3840 | | |
3765 | 3841 | | |
3766 | 3842 | | |
3767 | 3843 | | |
3768 | | - | |
3769 | | - | |
3770 | | - | |
3771 | | - | |
3772 | | - | |
3773 | | - | |
| 3844 | + | |
| 3845 | + | |
3774 | 3846 | | |
3775 | | - | |
| 3847 | + | |
3776 | 3848 | | |
3777 | 3849 | | |
3778 | 3850 | | |
3779 | 3851 | | |
3780 | 3852 | | |
3781 | | - | |
| 3853 | + | |
| 3854 | + | |
| 3855 | + | |
3782 | 3856 | | |
3783 | 3857 | | |
3784 | 3858 | | |
| |||
3787 | 3861 | | |
3788 | 3862 | | |
3789 | 3863 | | |
3790 | | - | |
3791 | | - | |
3792 | | - | |
3793 | | - | |
3794 | | - | |
3795 | | - | |
3796 | | - | |
3797 | | - | |
3798 | | - | |
| 3864 | + | |
3799 | 3865 | | |
3800 | 3866 | | |
3801 | 3867 | | |
| |||
3806 | 3872 | | |
3807 | 3873 | | |
3808 | 3874 | | |
3809 | | - | |
3810 | | - | |
3811 | | - | |
3812 | | - | |
3813 | | - | |
3814 | | - | |
| 3875 | + | |
| 3876 | + | |
| 3877 | + | |
| 3878 | + | |
| 3879 | + | |
3815 | 3880 | | |
3816 | | - | |
3817 | 3881 | | |
3818 | | - | |
3819 | | - | |
3820 | | - | |
| 3882 | + | |
| 3883 | + | |
3821 | 3884 | | |
3822 | 3885 | | |
3823 | 3886 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
270 | 270 | | |
271 | 271 | | |
272 | 272 | | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
273 | 297 | | |
274 | 298 | | |
| 299 | + | |
275 | 300 | | |
276 | | - | |
| 301 | + | |
| 302 | + | |
277 | 303 | | |
278 | 304 | | |
279 | 305 | | |
| |||
654 | 680 | | |
655 | 681 | | |
656 | 682 | | |
657 | | - | |
| 683 | + | |
| 684 | + | |
| 685 | + | |
| 686 | + | |
| 687 | + | |
| 688 | + | |
| 689 | + | |
| 690 | + | |
658 | 691 | | |
659 | 692 | | |
660 | 693 | | |
| |||
0 commit comments