Date: Sun, 21 Mar 2021 14:15:19 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Amit Langote <amitlangote09@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
Message-ID: <20210321191519.GA4146@telsasoft.com>
References: <20210321182903.GD11765@telsasoft.com>
 <20210321190712.GA8502@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210321190712.GA8502@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20210321191519.GA4146%40telsasoft.com>
Precedence: bulk

On Sun, Mar 21, 2021 at 01:14:20PM -0500, Justin Pryzby wrote:
> On Sun, Mar 21, 2021 at 03:01:15PM -0300, Alvaro Herrera wrote:
> > > But note that it doesn't check if an existing constraint "implies" the new
> > > constraint - maybe it should.
> > 
> > Hm, I'm not sure I want to do that, because that means that if I later
> > have to attach the partition again with the same partition bounds, then
> > I might have to incur a scan to recheck the constraint.  I think we want
> > to make the new constraint be as tight as possible ...
> 
> The ATTACH PARTITION checks if any existing constraint impilies the (proposed)
> partition bounds, not just if constraints are equal.  So I'm suggesting to do
> the same here.

On Sun, Mar 21, 2021 at 04:07:12PM -0300, Alvaro Herrera wrote:
> On 2021-Mar-21, Justin Pryzby wrote:
> > On Sun, Mar 21, 2021 at 03:22:00PM -0300, Alvaro Herrera wrote:
> > > So if we do that on DETACH, what would happen on ATTACH?
> > 
> > Do you mean what happens to the constraint that was already there ?
> > Nothing, since it's not ours to mess with.  Checking ImpliedBy() rather than
> > equal() doesn't change that.
> 
> No, I meant what happens regarding checking existing values in the
> table: is the table scanned even if the partition constraint is implied
> by existing table constraints?

I'm still not sure we're talking about the same thing.

Your patch adds a CHECK constraint during DETACH CONCURRENTLY, and I suggested
that it should avoid adding it if it's redundant with an existing constraint,
even if not equal().

The current behavior (since v10) is this:

postgres=# ALTER TABLE p ATTACH PARTITION p1 FOR VALUES FROM (1)TO(2);
DEBUG:  partition constraint for table "p1" is implied by existing constraints
ALTER TABLE

And that wouldn't change, except the CHECK constraint would be added
automatically during detach (if it wasn't already implied).  Maybe the CHECK
constraint should be added without CONCURRENTLY, too.  One fewer difference in
behavior.

-- 
Justin