On 3/14/24 11:13, Peter Eisentraut wrote:
> 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).
>
Seems OK to me.
>>>> 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.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company