On Fri, Nov 21, 2025 at 10:54 AM Corey Huinker <[email protected]>
wrote:
>
>> some of the switch->default, default don't have ``break``.
>>
>
> The compiler doesn't require them, but I see that we do use them in a lot
> of places, so I'll incorporate this.
>
>
>>
>> + for (int i = 0; i < nitems; i++)
>> + {
>> + MVNDistinctItem *item =
>> parse_state.distinct_items->elements[i].ptr_value;
>>
>> exposing the ptr_value seems not a good idea, we can foreach_ptr
>> the attached patch using foreach_ptr.
>
>
> I didn't like this because it makes getting the index value harder, and
> the index value is needed in lots of places. Instead I used list_nth() and
> list_nth_int().
>
>
>
>> in function pg_ndistinct_in some errsave can change to ereturn.
>> (I didn't do this part, though).
>>
>
> -0.25
>
> There's something that I like about the consistency of errsave() followed
> by a return that makes it clear that "the function ends here" that I don't
> get from ereturn().
>
> What I would really like, is a way to generate unique (but translated)
> errdetail values, and move the errsave/ereturn to after the switch
> statement.
>
>
>>
>> + /*
>> + * The attnum cannot be zero a negative number beyond the number of the
>> + * possible expressions.
>> + */
>> + if (attnum == 0 || attnum < (0-STATS_MAX_DIMENSIONS))
>> + {
>> + errsave(parse->escontext,
>> + errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>> + errmsg("malformed pg_ndistinct: \"%s\"", parse->str),
>> + errdetail("Invalid \"%s\" element: %d.",
>> + PG_NDISTINCT_KEY_ATTRIBUTES, attnum));
>> + return JSON_SEM_ACTION_FAILED;
>> + }
>> This part had no coverage tests, so I added a few.
>>
>
> +1
>
>
>
>>
>>
>> as mentioned before
>> + errsave(parse->escontext,
>> + errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>> + errmsg("malformed pg_ndistinct: \"%s\"", parse->str),
>> + errdetail("The \"%s\" key must contain an array of at least %d "
>> + "and no more than %d attributes.",
>> + PG_NDISTINCT_KEY_NDISTINCT, 2, STATS_MAX_DIMENSIONS));
>> here PG_NDISTINCT_KEY_NDISTINCT, should be PG_NDISTINCT_KEY_ATTRIBUTES.
>>
>
> +1
>
> Did similar things to pg_dependencies.
>
>
A few small comments.
1. Minor typo fixes:
In pg_dependencies_in: "pg_dependencies parssing claims..." could be
corrected to "pg_dependencies parsing claims..."
In ndistinct_array_end: "must be an non-empty array" might be better as
"must be a non-empty array"
In the comment for dependencies_object_field_start: "depeendency" appears
to be a typo for "dependency"
2.Code maintainability suggestion:
I noticed the string "malformed pg_dependencies: "%s"" is used repeatedly
throughout the code. Would you consider defining this as a macro? This
could reduce duplication and make future updates easier.
3.Memory management observation:
Regarding item_attnum_list, while PostgreSQL's memory context mechanism
handles cleanup, explicitly freeing the allocated memory after use might
improve code clarity.
These are all minor points - the implementation looks solid overall. Thank
you for your work on this feature!
