MisterRaindrop commented on code in PR #1842: URL: https://github.com/apache/cloudberry/pull/1842#discussion_r3840218562
########## contrib/interconnect/sql/interconnect.sql: ########## @@ -83,5 +89,5 @@ DROP EXTENSION interconnect; -- start_ignore \! gpconfig -r shared_preload_libraries -\! gpstop -raiq +\! gpstop -rafq Review Comment: Fair objection — split out into **#1919** and reverted here in a720cf69798, so this PR no longer touches `contrib/interconnect` at all. It was here because that race is what made this PR's `ic-contrib` job fail, and the failure is destructive rather than local: `gpstop -raiq` skips the shutdown checkpoint, the next startup runs crash recovery, and while the postmaster is in `PM_RECOVERY` it rejects the connection `gpstart` makes to read the segment configuration. `gpstop -r` then exits CRITICAL, `psql` gives up at the following `\c`, the whole file is skipped, and the cluster is left with only the coordinator running — which is why `pg_trgm` and `indexscan` also failed after it in the same job. #1919 has the full chain and the before/after measurements. Reverted rather than dropped by rewriting the branch, so this thread stays anchored to the change it is about. **One consequence to expect:** `ic-contrib` here may go red again until #1919 lands. That failure is in `contrib/interconnect` and has nothing to do with this module — the library is not in `shared_preload_libraries` for that job, so it is never loaded there. ########## 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]
