Date: Mon, 30 Nov 2020 19:30:51 -0600
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: <20201201013051.GA24052@telsasoft.com>
References: <20201016221321.GA31903@alvherre.pgsql>
 <20201103235606.GA17495@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201103235606.GA17495@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Nov 03, 2020 at 08:56:06PM -0300, Alvaro Herrera wrote:
> Here's an updated version of this patch.
> 
> Apart from rebasing to current master, I made the following changes:
> 
> * On the first transaction (the one that marks the partition as
> detached), the partition is locked with ShareLock rather than
> ShareUpdateExclusiveLock.  This means that DML is not allowed anymore.
> This seems a reasonable restriction to me; surely by the time you're
> detaching the partition you're not inserting data into it anymore.

I don't think it's an issue with your patch, but FYI that sounds like something
I had to do recently: detach *all* partitions of various tabls to promote their
partition key column from timestamp to timestamptz.  And we insert directly
into child tables, not routed via parent.

I don't your patch is still useful, but not to us.  So the documentation should
be clear about that.

> * ALTER TABLE .. DETACH PARTITION FINALIZE now waits for concurrent old
> snapshots, as previously discussed. This should ensure that the user
> doesn't just cancel the wait during the second transaction by Ctrl-C and
> run FINALIZE immediately afterwards, which I claimed would bring
> inconsistency.
> 
> * Avoid creating the CHECK constraint if an identical one already
> exists.
> 
> (Note I do not remove the constraint on ATTACH.  That seems pointless.)
> 
> Still to do: test this using the new hook added by 6f0b632f083b.

tab complete?

> +        * Ensure that foreign keys still hold after this detach.  This keeps lock
> +        * on the referencing tables, which prevent concurrent transactions from

keeps locks or
which prevents

> +++ b/doc/src/sgml/ref/alter_table.sgml
> @@ -947,6 +950,24 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
>        attached to the target table's indexes are detached.  Any triggers that
>        were created as clones of those in the target table are removed.
>       </para>
> +     <para>
> +      If <literal>CONCURRENTLY</literal> is specified, this process runs in two
> +      transactions in order to avoid blocking other sessions that might be accessing
> +      the partitioned table.  During the first transaction,
> +      <literal>SHARE UPDATE EXCLUSIVE</literal> is taken in both parent table and

missing "lock"
taken *on* ?

> +      partition, and the partition is marked detached; at that point, the transaction

probably "its partition,"

> +      If <literal>FINALIZE</literal> is specified, complete actions of a
> +      previous <literal>DETACH CONCURRENTLY</literal> invocation that
> +      was cancelled or crashed.

say "actions are completed" or:

  If FINALIZE is specified, a previous DETACH that was cancelled or interrupted
  is completed.

> +			if (!inhdetached && detached)
> +				ereport(ERROR,
> +						(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +						 errmsg("cannot complete detaching partition \"%s\"",
> +								childname),
> +						 errdetail("There's no partial concurrent detach in progress.")));

maybe say "partially-complete" or remove "partial"

> +		 * the partition being detached?  Putting them on the partition being
> +		 * detached would be wrong, since they'd become "lost" after the but
after *that* ?

> +	 * Concurrent mode has to work harder; first we add a new constraint to the
> +	 * partition that matches the partition constraint, if there isn't a matching
> +	 * one already.  The reason for this is that the planner may have made
> +	 * optimizations that depend on the constraint.  XXX Isn't it sufficient to
> +	 * invalidate the partition's relcache entry?

Ha.  I suggested this years ago.
https://www.postgresql.org/message-id/20180601221428.GU5164@telsasoft.com
|. The docs say: if detaching/re-attach a partition, should first ADD CHECK to
|  avoid a slow ATTACH operation.  Perhaps DETACHing a partition could
|  implicitly CREATE a constraint which is usable when reATTACHing?

> +	 * Then we close our existing transaction, and in a new one wait for
> +	 * all process to catch up on the catalog updates we've done so far; at

processes

> +		 * We don't need to concern ourselves with waiting for a lock the
> +		 * partition itself, since we will acquire AccessExclusiveLock below.

lock *on* ?

> +	 * If asked to, wait until existing snapshots are gone.  This is important
> +	 * in the second transaction of DETACH PARTITION CONCURRENTLY is canceled:

s/in/if/

> +++ b/src/bin/psql/describe.c
> -			printfPQExpBuffer(&tmpbuf, _("Partition of: %s %s"), parent_name,
> -							  partdef);
> +			printfPQExpBuffer(&tmpbuf, _("Partition of: %s %s%s"), parent_name,
> +							  partdef,
> +							  strcmp(detached, "t") == 0 ? " DETACHED" : "");

The attname "detached" is a stretch of what's intuitive (it's more like
"detachING" or half-detached).  But I think psql should for sure show something
more obvious to users.  Esp. seeing as psql output isn't documented.  Let's
figure out what to show to users and then maybe rename the column that, too.

> +PG_KEYWORD("finalize", FINALIZE, UNRESERVED_KEYWORD, BARE_LABEL)

Instead of finalize .. deferred ?  Or ??

ATExecDetachPartition:
Doesn't this need to lock the table before testing for default partition ?

I ended up with apparently broken constraint when running multiple loops around
a concurrent detach / attach:

while psql -h /tmp postgres -c "ALTER TABLE p ATTACH PARTITION p1 FOR VALUES FROM (1)TO(2)" -c "ALTER TABLE p DETACH PARTITION p1 CONCURRENTLY"; do :; done&
while psql -h /tmp postgres -c "ALTER TABLE p ATTACH PARTITION p1 FOR VALUES FROM (1)TO(2)" -c "ALTER TABLE p DETACH PARTITION p1 CONCURRENTLY"; do :; done&

    "p1_check" CHECK (true)
    "p1_i_check" CHECK (i IS NOT NULL AND i >= 1 AND i < 2)