Re: Make attstattarget nullable - Mailing list pgsql-hackers

From Tomas Vondra
Subject Re: Make attstattarget nullable
Date
Msg-id d18d062b-43e2-47a2-acc3-dd8a69c8016e@enterprisedb.com
Whole thread Raw
In response to Re: Make attstattarget nullable  (Peter Eisentraut <peter@eisentraut.org>)
Responses Re: Make attstattarget nullable
List pgsql-hackers

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



pgsql-hackers by date:

Previous
From: Aleksander Alekseev
Date:
Subject: Re: Add the ability to limit the amount of memory that can be allocated to backends.
Next
From: Aleksander Alekseev
Date:
Subject: Re: CF entries for 17 to be reviewed