Re: [ppa-dev] Fwd: Patch for ALTER TABLE CLUSTER ON

[Karl O. P. <ko...@me...>]

Hi Philippe (and Robert?),

On 06/09/2013 04:34:00 AM, phb07 wrote:
> >>> As far as I can understand, it adds a capability to the alter
> table
> >>> functionality, namely to specify, change or remove a clustered
> >> index
> >>> definition.
> >>> It does the equivalent of the "ALTER TABLE CLUSTER ON ..." and
> >> "ALTER
> >>> TABLE SET WITHOUT CLUSTER" sql statements.

My initial code review is here:

https://github.com/kpinc/phppgadmin/commits/altertablecluster

It contains only minor nits.

Note that I've not yet executed _any_ code.  (!)  So, I've not
seen the user interface either.  I have thoughts regardless:

Just because 7.4 can't remove clustering does not mean that
the clustering option should be non-existent.  Why not
just disappear the choice to remove clustering from the
user interface?  If we do this is should be committed as
a separate patch, ideally along with the hasAlterTableNocluster()
definition in Postgres.php, so that it can be rolled back with 
a single patch revert when 7.4 is dropped.  Or maybe not,
there might be enough manual fussing involved with dropping
7.4 that it's not worth it.

Presenting a clustered index name of '' (the empty string)
as an item in the select list of indexes
may not be the most clear way to indicate the absence
of clustering or, especially, to indicate that clustering
should be removed.  An alternative, like a "No Clustering"
checkbox or a index name of '<NONE>' might be better.
But both of these choices have problems.  Using a checkbox
raises the question of what to do with the select list.
We could always hide the select list with javascript/jQuery
when the No Clustering (or just "Cluster"?) checkbox
is checked.  Reasonably painless, but extra complication.  
And using most anything but '' as a choice
in the select list can, possibly, conflict with an index name.

I kind of lean towards a checkbox.

Thoughts or comments on any of the above?

Regards,

Karl <ko...@me...>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein