Re: dropping column prevented due to inherited index

Hello,

On Thu, Oct 10, 2019 at 1:13 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Wed, Oct 09, 2019 at 06:36:35AM -0300, Alvaro Herrera wrote:
> > Right, something like that. Needs a comment to explain what we do and
> > how recursing=true correlates with addrs=NULL, I think. Maybe add an
> > assert.
>
> Yes, that would be a thing to do. So I have added more comments
> regarding that aspect, an assertion, and more tests with a partitioned
> table without any children, and an actual check that all columns have
> been dropped in the leaves of the partition tree. How does that look?

Thanks for the patch. I think we should test there being multiple
dependent indexes on the column being dropped; the patch currently
tests only one.

Comments on comments: ;)

+ *
+ * *addrs stores the object addresses of all the columns to delete in
+ * case of a recursive lookup on children relations.

I think this comment fails to make clear the state of addrs in a given
invocation of ATExecDropColumn(). Maybe the whole header comment
could be rewritten to explain this function's new mode of operation.
How about this:

/*
* Drops column 'colName' from relation 'rel' and returns the address of the
* dropped column. The column is also dropped (or marked as no longer
* inherited from relation) from the relation's inheritance children, if any.
*
* In the recursive invocations for inheritance child relations, instead of
* dropping the column directly (if to be dropped at all), its object address
* is added to 'addrs', which must be non-NULL in such invocations.
* All columns are dropped at the same time after all the children have been
* checked recursively.
*/

/*
+ * Initialize the location of addresses which will be deleted on a
+ * recursive lookup on children relations. The structure to store all the
+ * object addresses to delete is initialized once when the recursive
+ * lookup begins.
+ */
+ Assert(!recursing || addrs != NULL);
+ if (!recursing)
+ addrs = new_object_addresses();

Maybe this could just say:

/* Initialize addrs on the first invocation. */

+ /*
+ * The recursion is ending, hence perform the actual column deletions.
+ */

Maybe:

/* All columns to be dropped must now be in addrs, so drop. */

Thanks,
Amit