On 12.03.24 14:32, Tomas Vondra wrote:
> On 3/12/24 13:47, Peter Eisentraut wrote:
>> On 06.03.24 22:34, Tomas Vondra wrote:
>>> 0001
>>> ----
>>>
>>> 1) I think this bit in ALTER STATISTICS docs is wrong:
>>>
>>> - <term><replaceable
>>> class="parameter">new_target</replaceable></term>
>>> + <term><literal>SET STATISTICS { <replaceable
>>> class="parameter">integer</replaceable> | DEFAULT }</literal></term>
>>>
>>> because it means we now have list entries for name, ..., new_name,
>>> new_schema, and then suddenly "SET STATISTICS { integer | DEFAULT }".
>>> That's a bit weird.
>>
>> Ok, how would you change it? List out the full clauses of the other
>> variants under Parameters as well?
>
> I'd go with a parameter, essentially exactly as it used to be, except
> for adding the DEFAULT option. So the list would define new_target, and
> mention DEFAULT as a special value.
Ok, done that way (I think).
>>> 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.