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.
