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?
We have similar inconsistencies on other ALTER reference pages, so I'm
not sure what the preferred approach is.
> 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.
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.