Hi Peter,
I took a look at this patch today. I think it makes sense to do this,
and I haven't found any major issues with any of the three patches. A
couple minor comments:
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.
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?
3) I find it a bit tedious that making the stxstattarget field nullable
means we now have to translate NULL to -1 in so many places. I know why
it's that way, but it's ... not very convenient :-(
0002
----
1) I think InsertPgAttributeTuples comment probably needs to document
what the new tupdesc_extra parameter does.
0003
----
no comment, seems fine
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
