MisterRaindrop commented on code in PR #1842: URL: https://github.com/apache/cloudberry/pull/1842#discussion_r3840218872
########## contrib/datalake_fdw/src/iceberg_volume_fdw/iceberg_volume_fdw.c: ########## @@ -0,0 +1,157 @@ +/*------------------------------------------------------------------------- + * + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + * iceberg_volume_fdw.c + * Option validator for Iceberg volume foreign servers. + * + * IDENTIFICATION + * contrib/datalake_fdw/src/iceberg_volume_fdw/iceberg_volume_fdw.c + * + *------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "access/reloptions.h" +#include "am_iceberg/pg_iceberg_options.h" +#include "catalog/pg_foreign_data_wrapper.h" +#include "catalog/pg_foreign_server.h" +#include "catalog/pg_user_mapping.h" +#include "commands/defrem.h" +#include "common/dl_option_util.h" +#include "fmgr.h" +#include "iceberg_volume_fdw/iceberg_volume_option.h" + +PG_FUNCTION_INFO_V1(iceberg_volume_fdw_validator); + +static bool is_volume_server_option(const char *name); +static bool is_volume_user_mapping_option(const char *name); + +static bool +is_volume_server_option(const char *name) +{ + return strcmp(name, DATALAKE_ICEBERG_VOLUME_BASE_PATH) == 0 || + strcmp(name, DATALAKE_ICEBERG_VOLUME_ENDPOINT) == 0 || + strcmp(name, DATALAKE_ICEBERG_VOLUME_REGION) == 0 || + strcmp(name, DATALAKE_ICEBERG_VOLUME_PATH_STYLE_ACCESS) == 0; +} + +/* + * Every credential here is optional, so that ambient storage credentials -- an + * instance profile, a ticket cache -- remain a valid deployment choice. + */ +static bool +is_volume_user_mapping_option(const char *name) +{ + return strcmp(name, DATALAKE_ICEBERG_VOLUME_USERNAME) == 0 || + strcmp(name, DATALAKE_ICEBERG_VOLUME_AWS_ACCESS_KEY_ID) == 0 || + strcmp(name, DATALAKE_ICEBERG_VOLUME_AWS_SECRET_ACCESS_KEY) == 0 || + strcmp(name, DATALAKE_ICEBERG_VOLUME_AWS_SESSION_TOKEN) == 0; +} + +Datum +iceberg_volume_fdw_validator(PG_FUNCTION_ARGS) +{ + List *options = untransformRelOptions(PG_GETARG_DATUM(0)); + Oid catalog = PG_GETARG_OID(1); + ListCell *lc; + IcebergVolumeServerOptions server_options; + IcebergForeignVolumeOptions volume_options; Review Comment: Applied in 2adeb73da50, and your reasoning is the better part of it. The declarations sit about sixty lines above where the memsets were, with the option-name validation loop and two early returns in between, so anything added to that stretch that read the structs would have read them uninitialized. Zeroing at the declaration removes that possibility. Being precise about what it buys: **no existing test can tell the two forms apart.** Both parse functions assign every field they own — `get_string_option()` returns NULL when an option is absent, and `get_bool_option_ex()` writes both the value and its "was set" flag — so the memsets were belt-and-braces rather than load bearing, and nothing between the declarations and the parse calls touched the structs. So this is future safety, not a fix, and I would rather say that than imply it closed a live bug. `= {0}` is also the form the rest of the tree uses (over a hundred occurrences in `src/backend/commands` and `src/backend/catalog` alone), so it improves consistency too. The module has one other `memset`, in `pg_iceberg_options.c`, which clears an out parameter reached through a pointer — there is no declaration there to attach an initializer to, so I left it alone rather than have it look overlooked. Verified: zero compiler warnings (the module builds with `-Werror=uninitialized` and without `-Wextra`, so the initializer neither trips nor silences anything) and `installcheck` 3/3 on a three-segment cluster. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
