Re: dropping column prevented due to inherited index

Hello,

On Thu, Oct 10, 2019 at 1:13 PM Michael Paquier <michael@paquier.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