On Thu, Oct 10, 2019 at 02:56:32PM +0900, Amit Langote wrote:
> /*
> * 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.
> */
Sounds fine to me.
> + * 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. */
I would add "recursive" here, to give:
/* Initialize addrs on the first recursive invocation. */
> + /*
> + * The recursion is ending, hence perform the actual column deletions.
> + */
>
> Maybe:
>
> /* All columns to be dropped must now be in addrs, so drop. */
I think that it would be better to clarify as well that this stands
after all the child relations have been checked, so what about that?
"The resursive lookup for inherited child relations is done. All the
child relations have been scanned and the object addresses of all the
columns to-be-dropped are registered in addrs, so drop."
--
Michael