Re: Make attstattarget nullable

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.

> We have similar inconsistencies on other ALTER reference pages, so I'm
> not sure what the preferred approach is.
> 

Yeah, the other reference pages may have some inconsistencies too, but
let's not add more.

>> 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.

> Note that this patch matches the equivalent patch for attstattarget
> (4f622503d6d), which uses the same logic.  We could change it if we have
> a better idea, but then we should change both.
> 
>> 0002
>> ----
>>
>> 1) I think InsertPgAttributeTuples comment probably needs to document
>> what the new tupdesc_extra parameter does.
> 
> Yes, I'll update that comment.
> 

OK.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company