On Thu, Oct 10, 2019 at 4:53 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Oct 10, 2019 at 02:56:32PM +0900, Amit Langote wrote:
> > /* Initialize addrs on the first invocation. */
>
> I would add "recursive" here, to give:
> /* Initialize addrs on the first recursive invocation. */
Actually, the code initializes it on the first call (recursing is
false) and asserts that it must have been already initialized in a
recursive (recursing is true) call.
> > + /*
> > + * 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."
Okay, sure. Maybe it's better to write the comment inside the if
block, because if recursing is true, we don't drop yet.
Thoughts on suggestion to expand the test case?
Thanks,
Amit