Feedback-ID: ie0a040ee:Fastmail
Message-ID: <d1cb16ec-9af1-4657-81f2-865480d2bd6d@eisentraut.org>
Date: Tue, 20 Feb 2024 15:47:46 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Content-Language: en-US
To: Justin Pryzby <pryzby@telsasoft.com>,
 Michael Paquier <michael@paquier.xyz>
Cc: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>,
 Zhihong Yu <zyu@yugabyte.com>, pgsql-hackers@postgresql.org,
 Ashwin Agrawal <ashwinstar@gmail.com>, vanjared@vmware.com
References: <ZCIxFkgOGv64nYE7@paquier.xyz> <ZCJuW3GOfE/0eBjh@telsasoft.com>
 <ZCUZLiCeXq4Im7OG@telsasoft.com> <ZEccbkBYCMy42Rxt@telsasoft.com>
 <ZG8E6DggGkPyEPPx@paquier.xyz> <ZHfZxjcR0J9Ws7ze@telsasoft.com>
 <ZHiUKhhT71X4CjJl@paquier.xyz> <ZLgn3Ou4YpgP8Z8l@telsasoft.com>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <ZLgn3Ou4YpgP8Z8l@telsasoft.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/d1cb16ec-9af1-4657-81f2-865480d2bd6d%40eisentraut.org>
Precedence: bulk

On 19.07.23 20:13, Justin Pryzby wrote:
> On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote:
>> On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote:
>>> What do you think the comment ought to say ?  It already says:
>>>
>>> src/backend/catalog/heap.c-              * Make a dependency link to force the relation to be deleted if its
>>> src/backend/catalog/heap.c-              * access method is.
>>
>> This is the third location where we rely on the fact that
>> RELKIND_HAS_TABLE_AM() does not include RELKIND_PARTITIONED_TABLE, so
>> it seems worth documenting what we are relying on as a comment?  Say:
>>   * Make a dependency link to force the relation to be deleted if its
>>   * access method is.
>>   *
>>   * No need to add an explicit dependency for the toast table, as the
>>   * main table depends on it.  Partitioned tables have a table access
>>   * method defined, and RELKIND_HAS_TABLE_AM ignores them.
> 
> You said that this location "relies on" the macro not including
> partitioned tables, but I would say the opposite: the places that use
> RELKIND_HAS_TABLE_AM() and do *not* say "or relkind==PARTITIONED_TABLE"
> are the ones that "rely on" that...
> 
> Anyway, this updates various comments.  No other changes.

It would be helpful if this patch could more extensively document in its 
commit message what semantic changes it makes.  Various options of 
possible behaviors were discussed in this thread, but it's not clear 
which behaviors were chosen in this particular patch version.

The general idea is that you can set an access method on a partitioned 
table.  That much seems very agreeable.  But then what happens with this 
setting, how can you override it, how can you change it, what happens 
when you change it, what happens with existing partitions and new 
partitions, etc. -- and which of these behaviors are new and old.  Many 
things to specify.