On Tue, Jan 23, 2024 at 12:29 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 22.01.24 13:23, Ashutosh Bapat wrote:
> >> if (newdef->identity)
> >> {
> >> Assert(!is_partioning);
> >> /*
> >> * Identity is never inherited. The new column can have an
> >> * identity definition, so we always just take that one.
> >> */
> >> def->identity = newdef->identity;
> >> }
> >>
> >> Thoughts?
> >
> > That code block already has Assert(!is_partition) at line 3085. I
> > thought that Assert is enough.
>
> Ok. Maybe just rephrase that comment somehow then?
Please see refactoring patches attached to [1]. Refactoring that way
makes it unnecessary to mention "regular inheritance" in each comment.
Yet I have included a modified version of the comment in that patch
set.
>
> > There's another thing I found. The file isn't using
> > check_stack_depth() in the function which traverse inheritance
> > hierarchies. This isn't just a problem of the identity related
> > function but most of the functions in that file. Do you think it's
> > worth fixing it?
>
> I suppose the number of inheritance levels is usually not a problem for
> stack depth?
>
Practically it should not. I would rethink the application design if
it requires so many inheritance or partition levels. But functions in
optimizer like try_partitionwise_join() and set_append_rel_size() call
/* Guard against stack overflow due to overly deep inheritance tree. */
check_stack_depth();
I am fine if we want to skip this.
--
Best Wishes,
Ashutosh Bapat