dropping column prevented due to inherited index

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

dropping column prevented due to inherited index

Amit Langote
Hi,

Maybe I'm forgetting some dependency management discussion that I was
part of recently, but is the following behavior unintentional?

create table p (a int, b int, c int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (b);
create table p11 partition of p1 for values in (1);
create index on p (c);
alter table p drop column c;
ERROR:  cannot drop index p11_c_idx because index p1_c_idx requires it
HINT:  You can drop index p1_c_idx instead.

Dropping c should've automatically dropped the index p_c_idx and its
children and grandchildren, so the above complaint seems redundant.

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Amit Langote
On Wed, Sep 4, 2019 at 2:36 PM Amit Langote <[hidden email]> wrote:

>
> Hi,
>
> Maybe I'm forgetting some dependency management discussion that I was
> part of recently, but is the following behavior unintentional?
>
> create table p (a int, b int, c int) partition by list (a);
> create table p1 partition of p for values in (1) partition by list (b);
> create table p11 partition of p1 for values in (1);
> create index on p (c);
> alter table p drop column c;
> ERROR:  cannot drop index p11_c_idx because index p1_c_idx requires it
> HINT:  You can drop index p1_c_idx instead.
>
> Dropping c should've automatically dropped the index p_c_idx and its
> children and grandchildren, so the above complaint seems redundant.
Interestingly, this behavior only occurs with v12 and HEAD.  With v11:

create table p (a int, b int, c int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (b);
create table p11 partition of p1 for values in (1);
create index on p (c);
alter table p drop column c; -- ok

Note that the multi-level partitioning is not really needed to
reproduce the error.

I think the error in my first email has to do with the following
commit made to v12 and HEAD branches earlier this year:

commit 1d92a0c9f7dd547ad14cd8a3974289c5ec7f04ce
Author: Tom Lane <[hidden email]>
Date:   Mon Feb 11 14:41:13 2019 -0500

    Redesign the partition dependency mechanism.

There may not really be any problem with the commit itself, but I
suspect that the new types of dependencies (or the way
findDependentObject() analyzes them) don't play well with inheritance
recursion of ATExecDropColumn().  Currently, child columns (and its
dependencies) are dropped before the parent column (and its
dependencies).  By using the attached patch which reverses that order,
the error goes away, but I'm not sure that that's the correct
solution.

Tom, thoughts?

Thanks,
Amit

ATExecDropColumn-inh-recursion-fix.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Alvaro Herrera-9
On 2019-Oct-03, Amit Langote wrote:

> There may not really be any problem with the commit itself, but I
> suspect that the new types of dependencies (or the way
> findDependentObject() analyzes them) don't play well with inheritance
> recursion of ATExecDropColumn().  Currently, child columns (and its
> dependencies) are dropped before the parent column (and its
> dependencies).  By using the attached patch which reverses that order,
> the error goes away, but I'm not sure that that's the correct
> solution.

Hmm.  I wonder if we shouldn't adopt the coding pattern we've used
elsewhere of collecting all columns to be dropped first into an
ObjectAddresses array, then use performMultipleDeletions.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Michael Paquier-2
On Thu, Oct 03, 2019 at 09:18:12AM -0300, Alvaro Herrera wrote:
> Hmm.  I wonder if we shouldn't adopt the coding pattern we've used
> elsewhere of collecting all columns to be dropped first into an
> ObjectAddresses array, then use performMultipleDeletions.

+1.  That's the common pattern these days, because that's more
performant.  I think that the patch should have regression tests.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Amit Langote
Hello,

On Fri, Oct 4, 2019 at 5:57 PM Michael Paquier <[hidden email]> wrote:
>
> On Thu, Oct 03, 2019 at 09:18:12AM -0300, Alvaro Herrera wrote:
> > Hmm.  I wonder if we shouldn't adopt the coding pattern we've used
> > elsewhere of collecting all columns to be dropped first into an
> > ObjectAddresses array, then use performMultipleDeletions.
>
> +1.  That's the common pattern these days, because that's more
> performant.

Actually I don't see the peformMultipleDeletions() pattern being used
for the situations where there are multiple objects to drop due to
inheritance.  I only see it where there are multiple objects related
to one table.  Maybe it's possible to apply to the inheritance
situation though, but in this particular case, it seems a bit hard to
do, because ATExecDropColumn steps through an inheritance tree level
at a time.

But maybe I misunderstood Alvaro's suggestion?

>  I think that the patch should have regression tests.

I have added one in the attached updated patch.

Thanks,
Amit

ATExecDropColumn-inh-recursion-fix_v2.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Ashutosh Sharma
I think we could have first deleted all the dependency of child object
on parent and then deleted the child itself using performDeletion().
As an example let's consider the case of toast table where we first
delete the dependency of toast relation on main relation and then
delete the toast table itself otherwise the toast table deletion would
fail. But, the problem I see here is that currently we do not have any
entry in pg_attribute table that would tell us about the dependency of
child column on parent.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Mon, Oct 7, 2019 at 7:31 AM Amit Langote <[hidden email]> wrote:

>
> Hello,
>
> On Fri, Oct 4, 2019 at 5:57 PM Michael Paquier <[hidden email]> wrote:
> >
> > On Thu, Oct 03, 2019 at 09:18:12AM -0300, Alvaro Herrera wrote:
> > > Hmm.  I wonder if we shouldn't adopt the coding pattern we've used
> > > elsewhere of collecting all columns to be dropped first into an
> > > ObjectAddresses array, then use performMultipleDeletions.
> >
> > +1.  That's the common pattern these days, because that's more
> > performant.
>
> Actually I don't see the peformMultipleDeletions() pattern being used
> for the situations where there are multiple objects to drop due to
> inheritance.  I only see it where there are multiple objects related
> to one table.  Maybe it's possible to apply to the inheritance
> situation though, but in this particular case, it seems a bit hard to
> do, because ATExecDropColumn steps through an inheritance tree level
> at a time.
>
> But maybe I misunderstood Alvaro's suggestion?
>
> >  I think that the patch should have regression tests.
>
> I have added one in the attached updated patch.
>
> Thanks,
> Amit


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Amit Langote
Hi Ashutosh,

On Mon, Oct 7, 2019 at 1:39 PM Ashutosh Sharma <[hidden email]> wrote:
> I think we could have first deleted all the dependency of child object
> on parent and then deleted the child itself using performDeletion().

So, there are two objects to consider in this case -- column and an
index that depends on it.

For columns, we don't store any dependency records for the dependency
between a child column and its corresponding parent column.  That
dependency is explicitly managed by the code and the attinhcount flag,
which if set, prevents the column from being dropped on its own.

Indexes do rely on dependency records for the dependency between a
child index and its corresponding parent index.  This dependency
prevents a child index from being dropped if the corresponding parent
index is also not being dropped.

In this case, recursively dropping a child's column will in turn try
to drop the depending child index.  findDependentObject() complains
because it can't allow a child index to be dropped, because it can't
establish that the corresponding parent index is also being dropped.
That's because the parent index will be dropped when the parent's
column will be dropped, which due to current coding of
ATExecDropColumn() is *after* all the child columns and indexes are
dropped.  If we drop the parent column and depending index first and
then recurse to children as my proposed patch does, then the parent
index would already have been dropped when dropping the child column
and the depending index.

> As an example let's consider the case of toast table where we first
> delete the dependency of toast relation on main relation and then
> delete the toast table itself otherwise the toast table deletion would
> fail. But, the problem I see here is that currently we do not have any
> entry in pg_attribute table that would tell us about the dependency of
> child column on parent.

I couldn't imagine how that trick could be implemented for this case. :(

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Alvaro Herrera-9
Apologies for not helping much here; I'm on vacation for a couple of
weeks.

On 2019-Oct-08, Amit Langote wrote:

> I couldn't imagine how that trick could be implemented for this case. :(

Can't we pass around an ObjectAddresses pointer to which each recursion
level adds the object(s) it wants to delete, and in the outermost level
we drop everything in it?  I vaguely recall we implemented something
like that somewhere within the past year, but all I can find is
20bef2c3110a.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Amit Langote
On Tue, Oct 8, 2019 at 6:15 PM Alvaro Herrera <[hidden email]> wrote:
> Apologies for not helping much here; I'm on vacation for a couple of
> weeks.

No worries, please take your time.

> On 2019-Oct-08, Amit Langote wrote:
>
> > I couldn't imagine how that trick could be implemented for this case. :(
>
> Can't we pass around an ObjectAddresses pointer to which each recursion
> level adds the object(s) it wants to delete, and in the outermost level
> we drop everything in it?  I vaguely recall we implemented something
> like that somewhere within the past year, but all I can find is
> 20bef2c3110a.

I thought about doing something like that, but wasn't sure if
introducing that much complexity is warranted.

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Ashutosh Sharma
In reply to this post by Amit Langote
On Tue, Oct 8, 2019 at 2:12 PM Amit Langote <[hidden email]> wrote:

>
> Hi Ashutosh,
>
> On Mon, Oct 7, 2019 at 1:39 PM Ashutosh Sharma <[hidden email]> wrote:
> > I think we could have first deleted all the dependency of child object
> > on parent and then deleted the child itself using performDeletion().
>
> So, there are two objects to consider in this case -- column and an
> index that depends on it.
>
> For columns, we don't store any dependency records for the dependency
> between a child column and its corresponding parent column.  That
> dependency is explicitly managed by the code and the attinhcount flag,
> which if set, prevents the column from being dropped on its own.
>
> Indexes do rely on dependency records for the dependency between a
> child index and its corresponding parent index.  This dependency
> prevents a child index from being dropped if the corresponding parent
> index is also not being dropped.
>
> In this case, recursively dropping a child's column will in turn try
> to drop the depending child index.  findDependentObject() complains
> because it can't allow a child index to be dropped, because it can't
> establish that the corresponding parent index is also being dropped.
> That's because the parent index will be dropped when the parent's
> column will be dropped, which due to current coding of
> ATExecDropColumn() is *after* all the child columns and indexes are
> dropped.  If we drop the parent column and depending index first and
> then recurse to children as my proposed patch does, then the parent
> index would already have been dropped when dropping the child column
> and the depending index.
>
> > As an example let's consider the case of toast table where we first
> > delete the dependency of toast relation on main relation and then
> > delete the toast table itself otherwise the toast table deletion would
> > fail. But, the problem I see here is that currently we do not have any
> > entry in pg_attribute table that would tell us about the dependency of
> > child column on parent.
>
> I couldn't imagine how that trick could be implemented for this case. :(
>

I don't think that is possible because presently in pg_attribute table
we do not have any column indicating that there exists index on the
given attribute. If we were knowing that then we could find out if the
given index on child table have been inherited from parent and if so,
we could delete all the dependencies on the child table index first
and then delete the column itself in the child table but that doesn't
seem to be doable here. So, although the standard way that I feel to
perform an object deletion is to first remove all it's dependencies
from the pg_depend table and then delete the object itself but
considering the information available in the relevant catalog table
that doesn't seem to be possible.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Michael Paquier-2
In reply to this post by Amit Langote
On Tue, Oct 08, 2019 at 06:25:05PM +0900, Amit Langote wrote:
> I thought about doing something like that, but wasn't sure if
> introducing that much complexity is warranted.

I looked at that.  By experience, I think that it would be wiser to do
first the lookup of all the dependencies you would like to delete, and
then let the internal dependency machinery sort things out after
recursing (remember recent fixes related to ON COMMIT actions).  In
order to do that, you actually just need to be careful to not trigger
the deletions as long as "recursing" is true because ATExecDropColumn
calls itself.  And it is not actually as bad as I assumed, please see
the attached.
--
Michael

ATExecDropColumn-inh-recursion-fix_v3.patch (5K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Alvaro Herrera-9
On 2019-Oct-09, Michael Paquier wrote:

> On Tue, Oct 08, 2019 at 06:25:05PM +0900, Amit Langote wrote:
> > I thought about doing something like that, but wasn't sure if
> > introducing that much complexity is warranted.
>
> I looked at that.  By experience, I think that it would be wiser to do
> first the lookup of all the dependencies you would like to delete, and
> then let the internal dependency machinery sort things out after
> recursing (remember recent fixes related to ON COMMIT actions).  In
> order to do that, you actually just need to be careful to not trigger
> the deletions as long as "recursing" is true because ATExecDropColumn
> calls itself.  And it is not actually as bad as I assumed, please see
> the attached.

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.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Michael Paquier-2
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?
--
Michael

ATExecDropColumn-inh-recursion-fix_v4.patch (7K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Amit Langote
Hello,

On Thu, Oct 10, 2019 at 1:13 PM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Amit Langote
On Thu, Oct 10, 2019 at 4:53 PM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Michael Paquier-2
On Thu, Oct 10, 2019 at 05:28:02PM +0900, Amit Langote wrote:
> 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.

I have actually kept your simplified version.

> Okay, sure.  Maybe it's better to write the comment inside the if
> block, because if recursing is true, we don't drop yet.

Sure.

> Thoughts on suggestion to expand the test case?

No objections to that, so done as per the attached.  Does that match
what you were thinking about?
--
Michael

ATExecDropColumn-inh-recursion-fix_v5.patch (9K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Amit Langote
On Fri, Oct 11, 2019 at 4:16 PM Michael Paquier <[hidden email]> wrote:

> On Thu, Oct 10, 2019 at 05:28:02PM +0900, Amit Langote wrote:
> > 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.
>
> I have actually kept your simplified version.
>
> > Okay, sure.  Maybe it's better to write the comment inside the if
> > block, because if recursing is true, we don't drop yet.
>
> Sure.
>
> > Thoughts on suggestion to expand the test case?
>
> No objections to that, so done as per the attached.  Does that match
> what you were thinking about?

Thanks.  The index on b is not really necessary for testing because it
remains unaffected, but maybe it's fine.

Regards,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Michael Paquier-2
On Fri, Oct 11, 2019 at 04:23:51PM +0900, Amit Langote wrote:
> Thanks.  The index on b is not really necessary for testing because it
> remains unaffected, but maybe it's fine.

That's on purpose.  Any more comments?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropping column prevented due to inherited index

Alvaro Herrera-9
In reply to this post by Michael Paquier-2
On 2019-Oct-11, Michael Paquier wrote:

> + if (!recursing)
> + {
> + /*
> + * The resursing 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.
> + */
> + performMultipleDeletions(addrs, behavior, 0);
> + free_object_addresses(addrs);
> + }

Typo "resursing".  This comment seems a bit too long to me.  Maybe
"Recursion having ended, drop everything that was collected." suffices.
(Fits in one line.)

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


12