Em qua., 18 de jun. de 2025 às 07:29, Etsuro Fujita <etsuro.fuj...@gmail.com> escreveu:
> On Tue, Jun 17, 2025 at 11:03 PM Fujii Masao > <masao.fu...@oss.nttdata.com> wrote: > > On 2025/06/17 20:37, Ranier Vilela wrote: > > > Em ter., 17 de jun. de 2025 às 06:09, Etsuro Fujita < > etsuro.fuj...@gmail.com <mailto:etsuro.fuj...@gmail.com>> escreveu: > > > On Tue, Jun 17, 2025 at 2:38 PM Fujii Masao < > masao.fu...@oss.nttdata.com <mailto:masao.fu...@oss.nttdata.com>> wrote: > > > > > adjust_foreign_grouping_path_cost(root, pathkeys, > > > > > retrieved_rows, width, > > > > - > fpextra->limit_tuples, > > > > + > fpextra ? fpextra->limit_tuples : 0.0, > > > > > &disabled_nodes, > > > > > &startup_cost, &run_cost); > > > > > > > > I couldn't find a query that would reach this code path with > > > > fpextra == NULL, but I agree the current code is fragile. > > > > So I think it's a good idea to add the check before accessing > > > > the field. > > > > > > We get here only when called from add_foreign_ordered_paths() or > > > add_foreign_final_paths(), in which cases fpextra is always set, > so it > > > cannot be NULL. No? > > > > > > False. > > > > > > In the function *postgresGetForeignRelSize* there is one call, > > > where fpextra is NULL. > > > > I think Etsuro-san meant that the problematic code path is only reachable > > when estimate_path_cost_size() is called from > add_foreign_ordered_paths() or > > add_foreign_final_paths(), and in those cases, fpextra is guaranteed to > > be non-NULL. In other cases, such as postgresGetForeignRelSize(), > > fpextra can be NULL, but the code path in question isn't reached - for > example, > > because pathkeys is NIL. > > Right. Another thing you need to be careful about here is the input > relation’s RelOptKind. As asserted right above > adjust_foreign_grouping_path_cost() in the code path in question, the > RelOptKind to exercise the code path to is limited to > RELOPT_UPPER_REL. Since 1) we only call estimate_path_cost_size() to > RELOPT_UPPER_REL in add_foreign_grouping_paths(), > add_foreign_ordered_paths(), and add_foreign_final_paths(), and 2) we > call it without pathways in the first function and with pathways in > the latter two functions, the code path can only be exercised when we > call it from the latter two functions, in which cases, as you > mentioned, we always set fpextra in those functions before calling it, > so fpextra cannot be NULL. > Ok. I will not insist on this point. But I think this is unnecessary mental gymnastics. >From a maintainability point of view, it is harmful. It would be enough to protect the pointer, as is already done in the rest of the code, and future uses of this function would be much simpler. > > As I mentioned earlier, I haven't found a case where this actually causes > > a crash, so Etsuro-san's analysis seems valid. That said, I still think > > it's safer to guard against NULL by checking fpextra before accessing > > its fields, as is done elsewhere. > > Considering fpextra cannot be NULL, I think the proposed change is > something more than necessary. IMO I think it is more appropriate to > just add an assertion and a comment for that like the attached, to > avoid this kind of confusion. I think I should have done so when > committing this. > I disapprove of this change, for me it worsens readability. It is better to continue without any changes, then. But if there is consensus, go ahead. best regards, Ranier Vilela
