ClickHouse 代码评审深度指南：编译标志副作用、访问权限检查与协议规范同步

ClickHouse 代码评审深度指南：编译标志副作用、访问权限检查与协议规范同步

【免费下载链接】ClickHouseClickHouse® is a real-time analytics database management system项目地址: https://gitcode.com/GitHub_Trending/cli/ClickHouse

本文基于 ClickHouse 仓库中.claude/skills/review/references.md这一评审参考文档，系统讲解三类仅在少数 PR 上触发的深度评审流程：编译标志变更对全局副作用与浮点语义的影响、新表函数/系统表等表面（surface）的访问权限检查完备性，以及原生协议/原生格式的字节级变更与官方规范文档的同步要求。读完本文，你将掌握如何以可验证的方式审计一个表函数是否在每条路径上都执行了权限检查、如何区分"有测试"与"有负向用例"，以及如何判断一次 wire 格式变更是否必须同步更新规范文档。

背景：评审参考文档的定位

ClickHouse 的评审技能文档.claude/skills/review/references.md本身并不罗列一份完整的基线检查清单，而是只描述一小部分 PR 上才会触发的扩展评审流程。它明确了触发条件：只有当 diff 中出现对应触发特征时，才去阅读对应章节；基线的检查项清单位于SKILL.md中，由基线清单命名触发器并指向这里的详细流程。

这种"按需触发"的设计说明了一个重要事实：仓库的大部分评审工作可以在通用检查清单指导下完成，但以下三类问题——编译标志放宽数值语义、访问控制缺路径、wire 字节变更未同步规范——一旦出现就是正确性或兼容性问题（Blocker / Major），因此需要单独、可验证的评审程序。本文按该文档的三节骨架逐一展开，并补充仓库源码证据。

编译标志变更：验证"无代码依赖旧行为"这一隐含契约

触发条件与受影响的标志

当 diff 修改了会移除某个被文档化的副作用或放宽跨大量翻译单元的浮点行为的编译/构建标志时，触发本评审。文档列出的标志及其影响如下：

标志	被移除/放宽的行为
-fno-math-errno	libm 不再设置errno
-ffast-math/-ffp-contract=fast/-fassociative-math/-freciprocal-math/-fno-signed-zeros	FP 运算被重排、收缩、改变舍入 → 结果不可复现
-fno-trapping-math	浮点陷阱语义被放宽
严格别名（strict-aliasing）放宽	类型别名规则被放松

引入这类标志的 PR 隐含承诺是："没有代码依赖被移除的旧行为"。评审者要把这句话当作契约来验证，而不能用一次 grep 当作证据——一个匹配行只有在同时满足两个条件时才有意义：(a)它确实以该标志编译；(b)它确实依赖该标志所移除的具体行为，而不仅仅是提到了相关符号。

(a) 构建图层面：未参与编译的代码不可能受影响

即使一个符号出现在源码中，只要它不在受该标志影响的编译单元里，就与本次变更无关。文档列举的典型反例包括：

• rust_vendor/ cargo crate（Rust 代码不受 C++ 编译标志影响）；
• EXCLUDE_FROM_ALL排除的目标；
• 被禁用的ENABLE_*选项对应的代码；
• contrib 中未被编译的部分——例如某个被 vendored 的数据库只构建了它的客户端库，其服务端后端代码从未参与编译。

(b) 语义层面：真正的消费方形态因标志而异

文档特别强调：每个标志的"真实消费方"形态都不同，不能把一个标志的检查形状套用到另一个上。

以-fno-math-errno为例：受影响的行为是<math.h>调用之后的errno，因此大多数匹配行都是误报——

• 设置errno的代码（libm 自身的errno = EDOM/with_errno(...)、错误码表、return ERANGE）不是消费方；
• 只有数学调用之后读取errno的代码才是消费方；
• 而且如果该读取的"生产者"并不是本标志触及的（例如strtod/strtol/scanf、系统调用），则该读取不受影响。

对于影响可复现性的 FP 标志（-ffast-math/ 收缩 / 结合律 / 有符号零），情况完全不同：这里根本没有"设置者/读取者"之分。消费方是任何假设"位级一致结果"的代码：

• 跨平台 / 跨 ISA 的可复现性；
• 分布式合并 / 聚合的一致性；
• 已提交的测试 reference 结果。

这类场景下成本是被承认并被论证的，而不是被 grep 审计出来的。

作用域意图：标志是否会渗入 vendored 库

还需要检查作用域意图：如果在add_subdirectory(contrib ...)之前把标志追加到COMPILER_FLAGS，标志会传播进 vendored 库。contrib 覆盖可能是刻意的（速度提升往往来自那里），也可能是危险的，但无论如何它必须是一个有意识、被明说的决定，并为每个目标单独刻画例外情况。评审结论的定级规则是：

• 一个真实消费方悄悄丢失该行为 → 正确性/兼容性Blocker；
• 未声明的 contrib 级可复现性变更 → 至少Major。

访问与权限检查：完备性优先于粒度

触发条件

当 diff 出现以下情况时触发本评审：

• 新增、移动、删除或依赖某个访问检查（context->checkAccess、getAccess()->checkAccess*、checkAccessRights、AccessType::*、行策略、readonly /allow_ddl门）；
• 或者新增了一个结果派生于查询用户可能无权读取的对象的表面——例如建立在另一张表之上的表函数或引擎、系统表、内省函数、新的子列、新的DESCRIBE/EXPLAIN/SHOW输出。

核心原则一：先问"完备性"，再问"粒度"

评审要回答的第一个问题是"检查是否在每条路径上、在任何派生发生之前被触达？"，然后才是"检查形态是否正确"。

批评某个AccessType的选择、列粒度或严格度看起来像安全发现且很容易做到，但它把前一个问题留空了——而一条完全跳过检查的路径是更严重的缺陷。作者声称"严格检查是有意为之"只能回答形态问题，回答不了路径完备性问题。

核心原则二：入口清单（Entrypoint Inventory）

一个新表面拥有的入口远多于写检查时考虑的那一个。对于一个读取另一个 ClickHouse 对象的表函数，文档给出了五个必须逐一审计的入口：

1. ITableFunction::parseArguments— 最先检查这里。TableFunctionFactory::get会在任何访问检查之前调用它，因此它是最早的载体，也最容易被忽视——因为名字看起来只是在解析语法。参数解析通常不止于字面量：它会解析存储 ID（Context::resolveStorageID）、获取表（DatabaseCatalog::instance().getTable）、将其转换为期望的引擎并读取其元数据，把类型名或列类型存到表函数对象上。要顺着它委托的每个 helper 追下去——存储的getConfiguration是常见的一个。注意typeid_cast也会抛异常：storagePtrToTimeSeries会报告"命名表的引擎不是 TimeSeries"，这泄露了表的存在性以及关于其引擎的一个否定事实；而MergeTree系 helpers 会直接打印got: {}并点明引擎名。两者都是泄露，需要分别定级，但不能把较弱的那种当作无害。这里解析出的任何信息都会在后续入口的检查之前泄露。
2. ITableFunction::getActualTableStructure— 由DESCRIBE table_function(...)（InterpreterDescribeQuery）、CREATE TABLE ... AS table_function(...)（InterpreterCreateQuery）以及ITableFunction::execute触达；execute用它校验非空缓存列与真实结构一致：hasStaticStructure() && cached_columns == getActualTableStructure(...)（见 ITableFunction.cpp 的 execute 实现）。这个调用是比较的操作数，所以对于静态结构函数，只要 execute 带着缓存列运行，它就每次都会执行，无论比较结果如何——不要把它读成只在失配时走到的异常路径。当结构不是静态时，execute根本不调用它，而是返回一个通过executeImpl惰性解析的StorageTableFunctionProxy。getActualTableStructureWithAccess不覆盖这里：它只检查源访问（READ ON MYSQL、READ ON S3…，由存储引擎名派生而来，见 ITableFunction.cpp 的 checkSourceAccess）。一个读取参数中命名的 ClickHouse 表的函数必须在这里自己检查对该表的SELECT——框架里没有任何机制替它做这件事。
3. ITableFunction::executeImpl— 构造存储。即使函数声明了固定列列表，它仍然会在这里解析并校验源表，所以不要因为getActualTableStructure是静态的就断定read之前没有任何派生。要下钻到它调用的存储构造函数：放在那里的校验（例如对MergeTreeData的dynamic_cast及其BAD_ARGUMENTS）在表面构建期间就会执行，早于read中的任何检查——存储构造函数也是入口。
4. IStorage::read— 读取本身。
5. 任何绕过read回答对象问题的东西：totalRows、totalRowsByPartitionPredicate、totalBytes、totalBytesUncompressed、getQueryProcessingStage、trivial-count 优化、能力探测。
6. 变更操作（如果表面有的话）：INSERT INTO FUNCTION、TRUNCATE、ALTER、DROP、RENAME。添加了 guard 的类通常不会覆盖继承来的那些。

对于系统表，对应物是fillData和read中的逐行过滤；对于子列或内省函数，则是每一条能渲染该值的路径。

核心原则三：元数据与错误文本都是受保护信息

"没有数据泄露，读取已被检查"不是辩护。列名与类型、引擎名、part 名、大小、行数、存在性、甚至错误码本身都是关于对象的信息。因此检查必须在任何从对象派生信息之前运行：在校验之前、在dynamic_cast诊断之前、在结构派生之前、在任何一个消息中指名对象属性的异常之前。

例如，在权限检查之前抛出expected MergeTree table, got: Log，就告诉了一个无权 select 该表的用户其引擎类型；区分UNKNOWN_TABLE与ACCESS_DENIED同样会泄露存在性。要遵循周围代码的既有做法，但永远不要让新消息比守卫它的检查更具体。

核心原则四：测试——每个入口一个负向用例

要求权限是用户可见行为，所以每个被守卫的路径都需要自己的用例：

• 无授权时的读取；
• 无授权时的结构解析（DESCRIBE）；
• 必须仍然被拒绝的部分授权；
• 必须成功的完整授权。

一个只有"为它写的那个路径"的测试的检查，对其它路径什么也证明不了；一个没有测试的检查本身就构成 Major。

同族表面（Sibling Surfaces）：从注册处枚举，而不是从文件名

同一族的表面往往以彼此的模板为蓝本写成，因此一个表面的缺陷通常存在于全部表面中。要从注册处枚举整族——表函数是factory.registerFunction与registerAlias，其它等价物在各自的 registry——而不是从文件或类名出发，因为一个类可以在构造标志下注册成多个名字，而构造标志会改变它解析目标的方式。

在 ClickHouse 中，这个规则有真实的、可验证的例子：TableFunctionMergeTreeAnalyzeIndexes 以resolve_by_uuid构造标志被注册两次，分别暴露mergeTreeAnalyzeIndexes与mergeTreeAnalyzeIndexesUUID。UUID 变体通过DatabaseCatalog::tryGetByUUID按 UUID 定位表，从不调用resolveStorageID，参数中根本没有库名和表名——"检查用户命名的库和表上的 SELECT"这种措辞对它无的放矢，必须转而基于已解析存储自身的StorageID工作。两个注册都标了allow_readonly，所以execute中的CREATE_TEMPORARY_TABLE检查也不会为它们触发。

更隐蔽的例子是prometheusQuery：它注册在 TableFunctionTimeSeries.cpp 中，紧挨着timeSeries*函数，且类通过同样的getConfiguration模式解析其源——但它不共享名字词干，所以用 greptimeSeries驱动的同族扫描会同时漏掉两个变体。同族成员资格由代码形态定义，而不是名字前缀。

文档还给出了一个按注册而非文件进行的一次真实扫描示例（mergeTree*与timeSeries*表函数），展示了"检查坐在 read 上，而每个函数在那之前解析其源表"这一共同形态，源表在三个位置被解析：

• getActualTableStructure—mergeTreeIndex、mergeTreeProjection（TableFunctionMergeTreeProjection）与mergeTreeCodecBlockCounts，各自在返回前抛出：expected MergeTree table, got: {}、There is no projection {} in table {}；
• executeImpl—mergeTreeTextIndex与mergeTreeAnalyzeIndexes两兄弟（后者的getActualTableStructure是固定列列表，不触表）。mergeTreeTextIndex在此抛出Got index '{}' of type '{}', expected 'text'；analyze-indexes 这一对的泄露还要更深一层，在StorageMergeTreeAnalyzeIndexes构造函数里（expected MergeTree table, got: {}），由executeImpl调用——构建表面时调用的存储构造函数同样是入口；
• parseArguments—timeSeriesSamples（别名timeSeriesData）/timeSeriesMetrics/timeSeriesTags（TableFunctionTimeSeriesTarget，调用getTargetTable存储目标引擎名，泄露早于getActualTableStructure）、timeSeriesSelector（经StorageTimeSeriesSelector::getConfiguration）、prometheusQuery/prometheusQueryRange（经StoragePrometheusQuery::getConfiguration）。

读取侧的三种结局：直接检查、委托检查、丢失检查

这是文档中最值得学习的一节："这个表面是否检查"不是 grep 问题。读取侧有三种结局，而非两种：

1. 直接检查。每个mergeTree*函数都在自己的read/readImpl中显式命名源表并执行context->checkAccess(AccessType::SELECT, …)。例如StorageMergeTreeCodecBlockCounts::checkSourceTableAccess会对源表的所有物理列做SELECT检查（StorageMergeTreeCodecBlockCounts.cpp），resolveSourceTable还会先做SHOW_TABLES检查（第 316-332 行）。

2. 通过委托继承检查。timeSeriesSelector、prometheusQuery/prometheusQueryRange的readImpl会构造内部SELECTAST，其FROM是对 tags / samples 表的ASTTableIdentifier，并交给InterpreterSelectQueryAnalyzer执行。这些是普通TableNode，会走 PlannerJoinTree.cpp 的checkAccessRights分支，从而在目标表上获得真正的列级SELECT检查——两个存储自身都没有任何checkAccess调用。单独 grep 会把这类表面误判为未守卫。它们仍然从不检查参数中命名的源TimeSeries表上的SELECT。

3. 因返回真实存储而丢失检查。timeSeriesSamples（及别名timeSeriesData）/timeSeriesMetrics/timeSeriesTags的executeImpl返回目标表自身的存储而不是包装器，因此没有可继承检查的内部查询，外层节点是TableFunctionNode——两个规划器都刻意排除它："we do not check access rights for table functions because they have been already checked inITableFunction::execute"（PlannerJoinTree.cpp 第 795-797 行），InterpreterSelectQuery.cpp中还有对应的!joined_tables.isLeftTableFunction()。而execute只检查getSourceAccessObject——由引擎名派生，对于MergeTree目标而言为空。于是源与目标在任何路径上都不被检查。

两条教训：其一，"这个表面是否检查"不是 grep 能回答的——没有checkAccess的存储可能因委托给内部查询而完全被守卫（结局 2），而返回他人真实存储的存储可能恰恰因为框架相信表函数已经检查过而完全不设防（结局 3）。其二，框架对表函数节点的排除意味着ITableFunction::execute是规划器假设存在的唯一守卫——因此任何在参数中命名 ClickHouse 表的表函数必须自己检查SELECT，结局 3 展示了当表面派生自真实表而无人检查时会发生什么。

严重度定级

• 在没有检查的情况下触达受保护数据或元数据的入口 →Blocker，即使没有行数据泄露；
• 仅因过粗而拒绝本应足够的授权的检查 → 最多Major，且在文档化并测试后是合理的设计选择。

完整实例：mergeTreeCodecBlockCounts的教训

文档以mergeTreeCodecBlockCounts为完整实例展示了"证据缺口"的通常形态。该函数最初只在StorageMergeTreeCodecBlockCounts::read中检查源表的SELECT。评审发现了这个检查并质疑其列粒度，作者辩护为有意为之，评审便停在了那里。然而TableFunctionMergeTreeCodecBlockCounts::getActualTableStructure直通DatabaseCatalog（见 TableFunctionMergeTreeCodecBlockCounts.cpp 第 77-84 行），于是对db.t无任何权限的用户也能DESCRIBE mergeTreeCodecBlockCounts(db, t)成功，且其BAD_ARGUMENTS消息泄露了表的引擎。

测试情况需要精确陈述：这个函数并不缺测试——仓库中现存十个.sql文件（04267_mergeTreeCodecBlockCounts_basic.sql到04627_mergeTreeCodecBlockCounts_no_substream_marks.sql），且两条代码路径都被覆盖：04267在结构路径上命中非MergeTree的BAD_ARGUMENTS，04509断言 read 时的ACCESS_DENIED（04509_mergeTreeCodecBlockCounts_row_policy.sql）。但它们没有一个以缺乏授权的用户身份运行：没有一个创建用户或授予任何权限，也没有一个发出DESCRIBE。所以read中的checkAccess(AccessType::SELECT, …)本身是未测试的——04509覆盖的是它旁边的行策略分支（一个不同的守卫）——套件里没有任何东西能区分"一个入口上的检查"与"所有入口上的检查"。缺口从来不是"没有测试"，而是"没有负向用例"，这正是上文"每个被守卫入口一个负向用例"规则的由来。

文档同时指出，结构路径的修复在当时仍是打开的 PR，因此master上该路径未设防——这也是该函数出现在同族清单里而非作为已闭合案例的原因。读者应把这条历史教训当作方法论模板，而不是当前状态的陈述。

原生协议 / 原生格式规范同步

触发条件

本评审仅在 diff 改变线上字节时触发：新增或修改了包类型或握手字段、DBMS_TCP_PROTOCOL_VERSION/DBMS_MIN_REVISION_*升级、或NativeReader/NativeWriter中新增/修改了列编码。不改变发送内容的纯重构、保持布局的 bug 修复、以及TCPHandler/Connection的编辑都不触发。

检查要求

触发时，必须检查配套规范是否同步更新：

• 协议 → docs/reference/interfaces/specs/NativeProtocol.mdx（覆盖包帧、连接生命周期、版本协商与每个非Block消息体）；
• 格式 → docs/reference/interfaces/specs/NativeFormat.mdx（覆盖 Block 内的字节：wire 原语、列族、每种数据类型的编码与压缩帧）。

第三方客户端是照着这两份规范构建的，因此缺漏的更新是一个Major，并且发现中必须指名具体改了什么。这两份规范是成对发布的：协议页负责包与传输层，格式页负责Data族包内的字节，两者在各自文档中互引（NativeProtocol.mdx 的 Overview 明言了这种分工与"二进制、位置化、小端、单连接单查询"的协议属性）。

把评审方法论落回日常实践

综合三节内容，可以从该参考文档提炼出一套可复用的操作顺序：

1. 先定触发，再读章节——diff 里没有对应特征时，不要为这三类流程投入深度评审；
2. 编译标志——先排除未参与编译的代码（构建图），再按标志特有的语义形态甄别真实消费方，最后确认 contrib 覆盖是有意且被声明的；
3. 访问检查——按"完备性 → 粒度 → 测试 → 同族"的顺序推进：枚举每个入口（parseArguments→getActualTableStructure→executeImpl→ 存储构造函数 →read→ 元数据探测 → 变更操作），确保检查发生在任何派生之前，为每个守卫路径补一个无授权的负向用例，并从注册处枚举同族表面（同一类可注册多个名字、不同入口可能泄露于不同位置）；
4. 协议规范——任何 wire 字节变更都对照 NativeProtocol.mdx 与 NativeFormat.mdx 检查同步，缺更新即为 Major。

这套方法论的共同底色是：可验证性优先于直觉。grep 命中不是证据、代码路径被"读到过"不是覆盖、修复了单个入口不等于守住了全部入口。当评审结论需要支撑 Blocker 或 Major 定级时，能引用具体的入口函数、具体的测试文件与具体的检查调用链，才是这份参考文档期望的评审产出。

【免费下载链接】ClickHouseClickHouse® is a real-time analytics database management system项目地址: https://gitcode.com/GitHub_Trending/cli/ClickHouse