Feedback-ID: ia2694551:Fastmail
Date: Thu, 18 Apr 2024 18:17:56 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Justin Pryzby <pryzby@telsasoft.com>,
	Alexander Lakhin <exclusion@gmail.com>,
 Peter Eisentraut <peter@eisentraut.org>,
	Soumyadeep Chakraborty <soumyadeep2007@gmail.com>,
 Zhihong Yu <zyu@yugabyte.com>, pgsql-hackers@postgresql.org,
	Ashwin Agrawal <ashwinstar@gmail.com>, vanjared@vmware.com
Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Message-ID: <202404181617.fkow5ghqjcqs@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZiBsYJr9yC7rySHV@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/202404181617.fkow5ghqjcqs%40alvherre.pgsql>
Precedence: bulk

On 2024-Apr-18, Michael Paquier wrote:

> On Wed, Apr 17, 2024 at 10:31:52AM +0200, Alvaro Herrera wrote:

> > Hmm, maybe we should do a RESET of default_table_access_method before
> > printing the CREATE TABLE to avoid the confusion.
> 
> A hard reset would make the business around currTableAM that decides
> when to generate the SET default_table_access_method queries slightly
> more complicated, while increasing the number of queries run on the
> server.

Hmm, okay.  (I don't think we really care too much about the number of
queries, do we?)

> Fine by me to use two routines to generate the two different commands.
> I am finishing with the attached for now, making dumps, restores and
> upgrades work happily as far as I've tested.

Great.

> I was also worrying about a need to dump the protocol version to be
> able to track the relkind in the toc entries, but a45c78e3284b has
> already done one.  The difference in AM handling between relations
> without storage and relations with storage pushes the relkind logic
> more within the internals of pg_backup_archiver.c.

Hmm, does this mean that every dump taking since a45c78e3284b (April
1st) and before this commit will be unrestorable?  This doesn't worry me
too much, because we aren't even in beta yet ... and I think we don't
have a strict policy about it.

> --- a/src/bin/pg_dump/t/002_pg_dump.pl
> +++ b/src/bin/pg_dump/t/002_pg_dump.pl
> @@ -4591,11 +4591,9 @@ my %tests = (
>  			CREATE TABLE dump_test.regress_pg_dump_table_am_child_2
>  			  PARTITION OF dump_test.regress_pg_dump_table_am_parent FOR VALUES IN (2);',
>  		regexp => qr/^
> -			\QSET default_table_access_method = regress_table_am;\E
> -			(\n(?!SET[^;]+;)[^\n]*)*
> -			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_parent (\E
> -			(.*\n)*
>  			\QSET default_table_access_method = heap;\E
> +			(.*\n)*
> +			\QALTER TABLE dump_test.regress_pg_dump_table_am_parent SET ACCESS METHOD regress_table_am;\E
>  			(\n(?!SET[^;]+;)[^\n]*)*
>  			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_1 (\E
>  			(.*\n)*

This looks strange -- why did you remove matching for the CREATE TABLE
of the parent table?  That line should appear shortly before the ALTER
TABLE SET ACCESS METHOD for the same table, shouldn't it?  Maybe your
intention was to remove only the SET default_table_access_method
= regress_table_am line ... but it's not clear to me why we have the
"SET default_table_access_method = heap" line before the ALTER TABLE SET
ACCESS METHOD.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)