On 14.03.24 15:46, Tomas Vondra wrote:
>>>>> 2) The newtarget handling in AlterStatistics seems rather confusing.
>>>>> Why
>>>>> does it get set to -1 just to ignore the value later? For a while I was
>>>>> 99% sure ALTER STATISTICS ... SET STATISTICS DEFAULT will set the field
>>>>> to -1. Maybe ditching the first if block and directly checking
>>>>> stmt->stxstattarget before setting repl_val/repl_null would be better?
>>>>
>>>> But we also need to continue accepting -1 for default on input. The
>>>> current code achieves that, the proposed variant would not.
>>>
>>> OK, I did not realize that. But then maybe this should be explained in a
>>> comment before the new "if" block, because people won't realize why it
>>> needs to be this way.
>>
>> In the new version, I tried to write this more explicitly, and updated
>> tablecmds.c to match.
>
> WFM. It still seems a bit hard to read, but I don't know how to do it
> better. I guess it's how it has to be to deal with multiple default
> values in a backwards-compatible way. Good thing is it's localized in
> two places.
I have committed this patch series. Thanks.