Hi Michael-san, I know you committed the patches already, but...
On Tue, Sep 1, 2026 at 5:17 PM Michael Paquier <[email protected]> wrote: > Now, the real deal: > > + NullableDatum unused = {.isnull = true, .value = (Datum) 0}; > [...] > + args[ATTRELSCHEMA_ARG] = unused; > + args[ATTRELNAME_ARG] = unused; > + args[ATTNAME_ARG] = unused; > + args[ATTNUM_ARG] = unused; > + args[INHERITED_ARG] = unused; > > I'm finding this part of the patch not acceptable, because it is > dictated by the fact that import_attribute_statistics() does not care > about these five unused parameters for postgres_fdw, these arguments > being required for the restore functions of the relation and attribute > stats. The reason why I set up those parameters was that I thought we might use them in attribute_statistics_update_internal() in the future even when called from import_attribute_statistics(). Consider eg, adding to the former function a feature for stats-logging or anything that takes as an input a NullableDatum array that is assumed to store those parameters as well. > To me, this points to a design defect of the postgres_fdw code, > because we pass to the import function pointers for each value from a > fcinfo then rebuild one. That's wasteful, and it complicates the > interfaces. Instead of a positional array, I think that we should use > two dedicated structures with named fields instead (one for > pg_class/rels, one for atts/pg_statistic), for relations and > attributes to avoid the guesses with the elements that may or may not > be used (aka the hardcoded unused pieces are not welcome here). That > would give for the attributes something among the lines of: > typedef struct AttStatsValues > { > NullableDatum null_frac; > NullableDatum avg_width; > /* And the rest, should be around a dozen in total */ > } AttStatsValues; -1 from me. As I said in the original thread, import_attribute_statistics() should have individual arguments for each stats value, to fit error messages in stats-checking functions in stat_utils.c like this: ereport(WARNING, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("argument \"%s\" must be specified when argument \"%s\" is specified", arginfo[nullarg].argname, arginfo[otherarg].argname))); Also, we have exported import_attribute_statistics() in v19, so we should avoid changing its signature. So I think the second patch should be reverted at least. Sorry, but this isn't that trivial a change, so I think you should have taken more time (at least a few days) for others to look at it. > With that, we should be able to bypass the positional issues, as well > as the fact that some of the parameters are not used, while cleaning > up the FDW-side import functions and all their arguments. The point > is where to make the cut due to the pairing of the arguments from the > fcinfos in the restore functions, but that's doable. My way of thinking is the exact opposite of yours (and Corey's): I think it's better to handle both restore and FDW cases in a unified way, rather than complicating the code for the latter case, which also makes the version diff large and thus makes back-patching hard. Thanks for working on this! Best regards, Etsuro Fujita
