Re: Make attstattarget nullable

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